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

[Kadir Cetinkaya via Phabricator via cfe-commits]

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