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

Duncan P. N. Exon Smith via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Nov 29 13:36:07 PST 2021

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.)

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list