kadircet marked 15 inline comments as done. kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/Preamble.cpp:278 + // This shouldn't coincide with any real file name. + PP.PatchFileName = llvm::formatv("{0}_preamble_patch.h", FileName); + ---------------- sammccall wrote: > I'm slightly nervous about incorporating the filename itself, not sure why > but it feels unneccesary. > WDYT about "dir/__preamble_patch__.h"? it was done to get rid of path manipulations, but not that important. ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:281 + // We are only interested in newly added includes. + llvm::StringSet<> ExistingIncludes; + for (const auto &Inc : Preamble.LexedIncludes) ---------------- sammccall wrote: > Why not a DenseSet<pair<PPKeywordKind, StringRef>>? > (The copies probably don't matter, but I think it'd be a bit clearer and more > typesafe) PPKeywordKind didn't have a DenseMapInfo, adding one. It already has two invalid enum values. ================ Comment at: clang-tools-extra/clangd/Preamble.h:74 + /// ones in disabled regions. + std::vector<Inclusion> LexedIncludes; }; ---------------- sammccall wrote: > Since computing these inclusions seems to be cheap (you've disabled all the > IO), would it be clearer and more flexible to store the preamble text as a > string and compute both the baseline & new includes at the same time? > I'm thinking if other preamble constructs (macros?) are added, needing to put > more data structures in PreambleData, where if you store the text only those > can be private details. > > (In fact the preamble text is already stored in PrecompiledPreamble, you > could just expose it there) > > since a preamble can be used with multiple preamble patches i wanted to reduce lexing overhead. I suppose we can address that later if it shows up in the traces. exposing the contents in PrecompiledPreamble. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77392/new/ https://reviews.llvm.org/D77392 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits