[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles

[Sam McCall via Phabricator via cfe-commits]

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