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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
