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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf72027233044: [clang][lex] Include tracking: simplify and move to preprocessor (authored by jansvoboda11). Repository: rG LLVM Github Monorepo

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

vsapsai accepted this revision. vsapsai added a comment. This revision is now accepted and ready to land. Looks good to me. Thanks for working on it! Though I haven't built it locally and it can be useful for pre-merge checks to finish. Repository: rG LLVM Github Monorepo CHANGES SINCE

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

jansvoboda11 added a comment. In D114095#3263086 , @vsapsai wrote: > My understanding was that we were going with ID vectors approach and not with > bitvectors, and the current code still uses bitvectors. Also I have a minor > readability concern about

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

jansvoboda11 updated this revision to Diff 402968. jansvoboda11 added a comment. 1. Move `HeaderSearch::getFileInfo()` call into `Preprocessor::markIncluded()`. 2. Switch from bitvector to vector of file IDs in AST files. 3. Remove unnecessary usages of `auto`. Repository: rG LLVM Github

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

vsapsai added a comment. In D114095#3261073 , @jansvoboda11 wrote: > @vsapsai do you have any further concerns? My only intended change at this > point is Duncan's suggestion. My understanding was that we were going with ID vectors approach and not

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

jansvoboda11 added a comment. @vsapsai do you have any further concerns? My only intended change at this point is Duncan's suggestion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114095/new/ https://reviews.llvm.org/D114095

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

vsapsai added a comment. In D114095#3188557 , @jansvoboda11 wrote: > Given that, I think we should commit this patch with ID vectors, even though > in isolation (without D112915 ) it's the > worse solution. WDYT? I

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

jansvoboda11 added a comment. You're right, I measured only this patch, not per-submodule include tracking (D112915 ). With per-submodule tracking, the results look like this:

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

vsapsai added a comment. Thanks for measuring the .pcm impact! And I appreciate including the trunk baseline. The results aren't exactly the same I've seen before, so please forgive me my suspiciousness. When you mention > current trunk with this patch do you mean "just this D114095

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

jansvoboda11 added a comment. In D114095#3160103 , @vsapsai wrote: > I've mentioned it in D112915 as we've > discussed the stored data format there. But my concern was that bitvector > packing might be not the most

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

vsapsai added a comment. I've mentioned it in D112915 as we've discussed the stored data format there. But my concern was that bitvector packing might be not the most space-efficient encoding. I haven't done proper testing, just off-the-cuff comparison and it

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

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

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

jansvoboda11 updated this revision to Diff 388814. jansvoboda11 added a comment. Move record ID from `PREPROCESSOR_BLOCK` to `AST_BLOCK`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114095/new/ https://reviews.llvm.org/D114095 Files:

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

jansvoboda11 added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:873 RECORD(PP_TOKEN); + RECORD(PP_INCLUDED_FILES); vsapsai wrote: > I believe `PP_INCLUDED_FILES` is located in `AST_BLOCK`. Yes, you write it in >

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

vsapsai added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:873 RECORD(PP_TOKEN); + RECORD(PP_INCLUDED_FILES); I believe `PP_INCLUDED_FILES` is located in `AST_BLOCK`. Yes, you write it in `ASTWriter::WritePreprocessor` but after

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

jansvoboda11 marked an inline comment as done. jansvoboda11 added inline comments. Comment at: clang/lib/Lex/Preprocessor.cpp:552 +if (const FileEntry *FE = SourceMgr.getFileEntryForID(MainFileID)) { + HeaderInfo.getFileInfo(FE); + markIncluded(FE);

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

jansvoboda11 updated this revision to Diff 388407. jansvoboda11 added a comment. Add new record into block info. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114095/new/ https://reviews.llvm.org/D114095 Files:

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

vsapsai added inline comments. Comment at: clang/include/clang/Serialization/ASTBitCodes.h:700 + /// Record code for included files. + PP_INCLUDED_FILES = 66, }; Small detail. Do you need to add the new record to `ASTWriter::WriteBlockInfoBlock`? It's not

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

vsapsai added a comment. Haven't checked the details of serialization/deserialization. High-level question about different serialization approaches (bitvector vs list of file ids) is in D112915 . Comment at: