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

Iain Sandoe via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sat Mar 12 15:18:42 PST 2022


iains added inline comments.


================
Comment at: clang/include/clang/Frontend/FrontendOptions.h:157
+  unsigned HeaderUnit : 3;
+  unsigned Header : 1;
 
----------------
ChuanqiXu wrote:
> I prefer `IsHeader`
OK.


================
Comment at: clang/include/clang/Frontend/FrontendOptions.h:252
+  bool isHeader() const { return Kind.isHeader(); }
+  InputKind::HeaderUnitKind getHeaderUnit() const {
+    return Kind.getHeaderUnit();
----------------
ChuanqiXu wrote:
> How about `getHeaderUnitKind` ?
OK. I prefer more descriptive names, but they do get rather long sometimes.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2788-2791
+      HUK =
+          XValue.consume_back("-header-unit") ? InputKind::HeaderUnit_Abs : HUK;
+      HUK = XValue.consume_back("-system") ? InputKind::HeaderUnit_System : HUK;
+      HUK = XValue.consume_back("-user") ? InputKind::HeaderUnit_User : HUK;
----------------
ChuanqiXu wrote:
> How do you think about the suggested changes? I feel it is less confusing.
OK, 


================
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;
 
----------------
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.


================
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");
----------------
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.


================
Comment at: clang/lib/Frontend/FrontendAction.cpp:814-816
+      const DirectoryEntry *Dir = nullptr;
+      if (auto DirOrErr = CI.getFileManager().getDirectory("."))
+        Dir = *DirOrErr;
----------------
ChuanqiXu wrote:
> Is it same as the suggested?
not quite because getDirectory() is "llvm::ErrorOr<const DirectoryEntry *>" ... 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121096



More information about the cfe-commits mailing list