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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
