sammccall added a comment.
This looks pretty good! Haven't reviewed the tests or removal of consistent
preamble support yet. (Mostly note-to-self)
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:115
+ do {
+ PP.Lex(Tok);
+ } while (Tok.isNot(tok::eof) &&
----------------
Given your getDecomposedTok before, you might want to assert Tok is in the main
file inside the loop
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:126
+std::vector<Inclusion>
+getPreambleIncludes(llvm::StringRef Contents,
+ llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
----------------
I'd call this scanPreambleIncludes or something just slightly less generic -
again here we don't want it to be accidentally reused for something that needs
to be accurate.
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:148
+ PreambleOnlyAction Action;
+ if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]))
+ return {};
----------------
Something needs to check that there's one input etc.
(This could maybe be moved into buildCompilerInvocation I guess, but currently
the caller does it)
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:244
std::vector<Diag> Diags = PreambleDiagnostics.take();
+ auto AllIncludes = getPreambleIncludes(Inputs.Contents,
+
StatCache->getConsumingFS(Inputs.FS),
----------------
I'd stick to LexedIncludes or ApproxmimateIncludes or BaselineIncludes - it
excludes stuff under ifdefs that *can* be resolved, and we don't want it to be
used for other purposes...
================
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);
+
----------------
I'm slightly nervous about incorporating the filename itself, not sure why but
it feels unneccesary.
WDYT about "dir/__preamble_patch__.h"?
================
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)
----------------
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)
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:313
+ PPOpts.addRemappedFile(PatchFileName, PatchBuffer.release());
+ PPOpts.Includes.push_back(PatchFileName);
+}
----------------
worth a comment indicating exactly where this is going to be inserted into the
parsing stream.
================
Comment at: clang-tools-extra/clangd/Preamble.h:72
CanonicalIncludes CanonIncludes;
+ /// Contains all of the includes spelled in the preamble section, even the
+ /// ones in disabled regions.
----------------
Maybe "Used as a baseline for creating a PreamblePatch"?
Since this is a bit unusual.
================
Comment at: clang-tools-extra/clangd/Preamble.h:74
+ /// ones in disabled regions.
+ std::vector<Inclusion> LexedIncludes;
};
----------------
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)
================
Comment at: clang-tools-extra/clangd/Preamble.h:98
+/// Stores information required to use an existing preamble with different file
+/// contents.
+class PreamblePatch {
----------------
This is probably a good place for a high-level overview of the concept, and
maybe an example.
In particular, *something* should mention that this is approximate, and what we
use it for/don't use it for.
================
Comment at: clang-tools-extra/clangd/Preamble.h:101
+public:
+ PreamblePatch() = default;
+ /// Builds a patch that contains new PP directives introduced to the preamble
----------------
// With an empty patch, the preamble is used verbatim.
================
Comment at: clang-tools-extra/clangd/Preamble.h:107
+ static PreamblePatch create(llvm::StringRef FileName, const ParseInputs &PI,
+ const PreambleData &Preamble);
+ /// Inserts an artifical include to the \p CI that contains new directives
----------------
nit: it'd be good to give these "version"s names. e.g. PI -> Modified, Preamble
-> Baseline. And maybe mention these in the class description?
================
Comment at: clang-tools-extra/clangd/Preamble.h:108
+ const PreambleData &Preamble);
+ /// Inserts an artifical include to the \p CI that contains new directives
+ /// calculated with create.
----------------
This explanation is a little low-level, I'd add a first sentence explaining the
goal.
e.g. "Adjusts CI (which compiles the modified inputs) to be used with the
baseline preamble".
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
