This is an archive of the discontinued LLVM Phabricator instance.

[clang][lex] Refactor check for the first file include
ClosedPublic

Authored by jansvoboda11 on Nov 17 2021, 8:18 AM.

Details

Summary

This patch refactors the code that checks whether a file has just been included for the first time.

The HeaderSearch::FirstTimeLexingFile function is removed and the information is threaded to the original call site from HeaderSearch::ShouldEnterIncludeFile. This will make it possible to avoid tracking the number of includes in a follow up patch.

Depends on D114092.

Diff Detail

Event Timeline

jansvoboda11 requested review of this revision.Nov 17 2021, 8:18 AM
jansvoboda11 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2021, 8:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dexonsmith accepted this revision.Nov 17 2021, 12:43 PM

LGTM with one nit inline.

clang/include/clang/Lex/Lexer.h
143–144

Please move up next to the other bool declarations so that it uses the same word for storage.

This revision is now accepted and ready to land.Nov 17 2021, 12:43 PM

Have just a few little tweaks, otherwise looks good. And we've discussed that it's possible we don't really need the first time lexing check anymore. But that's a separate issue and I support your decision to preserve the existing behavior and not to increase the scope of work.

clang/include/clang/Lex/HeaderSearch.h
446–448

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

clang/include/clang/Lex/Preprocessor.h
1370–1371

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.

jansvoboda11 marked 3 inline comments as done.Nov 18 2021, 3:42 AM
jansvoboda11 added inline comments.
clang/include/clang/Lex/HeaderSearch.h
446–448

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.

clang/include/clang/Lex/Preprocessor.h
1370–1371

It's also called twice in Preprocessor::EnterMainSourceFile where specifying true would seem redundant. I think I'll keep it as is for now.

This revision was automatically updated to reflect the committed changes.
jansvoboda11 marked 2 inline comments as done.