[PATCH] D121096: [C++20][Modules][HU 2/5] Support searching Header Units in user or system search paths.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

LGTM except we should remove the dead error emitting.



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2796-2797
+    // not intended to be a module map or header unit.
+    IsHeaderFile = IsHeader && !Preprocessed && !ModuleMap &&
+                   HUK == InputKind::HeaderUnit_None;
 
----------------
iains wrote:
> ChuanqiXu wrote:
> > Oh, now I find `Header` and `HeaderUnit` might be a little bit confusing. It looks like `Header` should include `HeaderUnit` at the first sight. It is not true. But I don't have only suggestions...
> We have an amount of existing handling specific to "generic headers".
>  (header unit = none, header = true)
> 
> The intention is that HeaderUnitKind is used to disambiguate the cases we are dealing with C+=20 header units.
> 
> I am not sure it would be worth the code churn to replace all uses of header with a HUK.
OK, this is a historical problem. It makes no sense require us to fix it in this patch.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2855-2857
+  assert((DashX.getHeaderUnit() == InputKind::HeaderUnit_None ||
+          Inputs.size() == 1) &&
+         "Expected only one input file for header unit");
----------------
iains wrote:
> ChuanqiXu wrote:
> > So the compiler would crash if there is multiple inputs for header unit? I feel it is not most friendly. How about emitting an error here?
> well the driver is supposed to catch these problems (in the case of C++20 modules mode it will generate several compile jobs, one per header).
> 
> I added an error for this tho.
Oh, so if the compiler wouldn't crash in that case, I think the original assertion is acceptable. And the current error is dead code, which is bad. Let's take the original assertion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121096/new/

https://reviews.llvm.org/D121096