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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added a comment.

Great stuff!



================
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)
----------------
kadircet wrote:
> 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.
Ugh, I thought enums had those implicitly. The need for two invalid values is really annoying, it's probably why we don't have more implicit ones.

I wonder whether it's feasible (technically and legally) to replace DenseHashMap with a fork of absl::flat_hash_map. I'm pretty sure it's faster, and it doesn't have these weird API requirements.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:128
+std::vector<Inclusion>
+scanPreambleIncludes(llvm::StringRef Contents,
+                     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
----------------
The error-handling paths here just return {}, same as an empty preamble.

Results:
if both baseline & current fail, then we'll generate no patch --> fine
if baseline is ok and current fails, we'll consider all headers, we'll consider all headers removed and generate no new includes --> fine for now (added-includes-only model)
if baseline fails and current is OK, we'll create a patch that adds all the headers --> really slow, we'd be better off creating an empty patch

clarity and debuggability:
behaviors are implicit and silent. I think we should write out these three cases explicitly in the code, and log them at least at `-log/verbose`. These should be rare conditions I think. (If they're not, we should be able to track down & handle the common causes I think)

I think this function should probably return Expected.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77392/new/

https://reviews.llvm.org/D77392