[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 added inline comments.


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


================
Comment at: clang/include/clang/Frontend/FrontendOptions.h:252
+  bool isHeader() const { return Kind.isHeader(); }
+  InputKind::HeaderUnitKind getHeaderUnit() const {
+    return Kind.getHeaderUnit();
----------------
How about `getHeaderUnitKind` ?


================
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;
----------------
How do you think about the suggested changes? I feel it is less confusing.


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


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2838-2839
       DashX = DashX.getPreprocessed();
+    // A regular header is considered mutually exclusive with a header unit
+    // one
+    if (HUK != InputKind::HeaderUnit_None) {
----------------



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


================
Comment at: clang/lib/Frontend/FrontendAction.cpp:814-816
+      const DirectoryEntry *Dir = nullptr;
+      if (auto DirOrErr = CI.getFileManager().getDirectory("."))
+        Dir = *DirOrErr;
----------------
Is it same as the suggested?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121096