[PATCH] D114093: [clang][lex] Refactor check for the first file include

Thu Nov 18 03:42:29 PST 2021

jansvoboda11 marked 3 inline comments as done.
jansvoboda11 added inline comments.

================
Comment at: clang/include/clang/Lex/HeaderSearch.h:445-447
+  bool ShouldEnterIncludeFile(bool &IsFirstIncludeOfFile, Preprocessor &PP,
+                              const FileEntry *File, bool isImport,
+                              bool ModulesEnabled, Module *M);
----------------
vsapsai wrote:
> As we've discussed earlier, the order of parameters is slightly weird. Usually, out-parameters are at the end of the list, though I'm not sure there is a rule about it. Personally, I would move `File` to the front because the method is ShouldEnter**IncludeFile** and keep the order as `File, isImport, PP, ModulesEnabled, M, IsFirstIncludeOfFile` but that's more my personal opinion and I'll leave the final decision to you.
I agree the order is sometimes a bit weird. It seems like the convention is to pass "bigger" objects (like `Preprocessor`, `HeaderSearch`, `CompilerInstance` etc.) first. I'll move the new out param to the back, but probably won't mess with the existing parameters.

================
Comment at: clang/include/clang/Lex/Preprocessor.h:1370-1371
   /// Emits a diagnostic, doesn't enter the file, and returns true on error.
   bool EnterSourceFile(FileID FID, const DirectoryLookup *Dir,
-                       SourceLocation Loc);
+                       SourceLocation Loc, bool IsFirstIncludeOfFile = true);

----------------
vsapsai wrote:
> Not sure we need a default value for `IsFirstIncludeOfFile`. Grepping shows the only other place the method is called is `IncrementalParser::Parse`, so we can provide an explicit value there.
It's also called twice in `Preprocessor::EnterMainSourceFile` where specifying `true` would seem redundant. I think I'll keep it as is for now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114093