[PATCH] D114095: [clang][lex] Include tracking: simplify and move to preprocessor

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

This LGTM by the way (not sure if that was already clear, since you abandoned a different review than I anticipated when squashing, and I'd "accepted" the other one); also see my suggestion to move the call to getFileInfo inside of markIncluded (might be error prone not to make that change).

Haven't clicked "accept" here in case @vsapsai has more comments.



================
Comment at: clang/lib/Lex/Preprocessor.cpp:552
+    if (const FileEntry *FE = SourceMgr.getFileEntryForID(MainFileID)) {
+      HeaderInfo.getFileInfo(FE);
+      markIncluded(FE);
----------------
jansvoboda11 wrote:
> vsapsai wrote:
> > Why do you need to `getFileInfo` but don't use it? I have no objections but it looks like it deserves a comment because it's not obvious.
> Without the call, I'm hitting some assertions when running C++20 modules tests:
> 
> ```
> Assertion failed: (CurDiagID == std::numeric_limits<unsigned>::max() && "Multiple diagnostics in flight at once!"), function Report, file Diagnostic.h, line 1526.
> ```
> 
> ```
> fatal error: error in backend: -verify directives found after rather than during normal parsing of <llvm>/clang/test/CXX/modules-ts/basic/basic.def.odr/p6/global-vs-module.cpp
> ```
> 
> Might need to investigate more to be able to write up a reasonable comment here.
Not sure precisely why that assertion fires, but the call to `getFileInfo` makes sense since it's mutating -- it adds a HeaderFileInfo entry (and `IncrementIncludeCount()` used to call it). There are code paths that call `getExistingFileInfo` that will expect it to have been created on inclusion.

I suggest moving the `getFileInfo()` call to inside `markIncluded()` and adding a comment that says "Create the HeaderFileInfo if it doesn't already exist" or something. Parting of marking it included should be to ensure that callers of HeaderSearch::getExistingFileInfo get something back.

(This'd be more obvious (and the comment unnecessary) if getFileInfo were renamed to getOrCreateFileInfo, after which getExistingFileInfo could be renamed to getFileInfo.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114095