sammccall marked 5 inline comments as done.
sammccall added a comment.
In D80296#2053189 <https://reviews.llvm.org/D80296#2053189>, @adamcz wrote:
> Correct me if I'm wrong, but this might lead to a file never being updated in
> the index, even after edit, right?
> Let's say you have file a.cpp that includes a.h. a.h has
> namespace foo {
>
> #define EXPAND(foo) whatever
> #include "b.h"
>
> }
> all nodes from b.h will be inside the namespace node in a.h. If a.h did not
> change, b.h will never be indexed.
Yes, there's some potential regression here. It exists if:
- the including file is unmodified, the included is modified
- the including file defines the top-level decl (e.g. enclosing namespaces),
otherwise change has no effect
- the symbols are considered part of the included file for sharding purposes,
otherwise this already didn't work. (I think they are in most cases)
I think I'm comfortable throwing this in the "clangd is designed for
well-structured codebases" bucket, along with non-self-contained files in other
contexts (b.h is necessarily not self-contained here). I'm also comfortable
saying tablegen isn't a well-structured part of LLVM :-)
And for preamble index, (which doesn't yet have this optimization) I think
that's the end of the story.
However, this being persistently sticky *forever* with the background index
does seem pretty horrible. I can't see an elegant fix though, and a 33% speedup
for background index is an important win (it's a top clangd complaint).
Any bright ideas? If not, maybe we land this and try to fix it somehow later,
or just accept it as one of our speed/accuracy tradeoffs.
================
Comment at: clang-tools-extra/clangd/unittests/IndexActionTests.cpp:22
using ::testing::ElementsAre;
+using testing::EndsWith;
using ::testing::Not;
----------------
kadircet wrote:
> nit `using ::testing::EndsWith`
>
> was this `add using` tweak ?
Yes, it was.
I feel a bit silly as I'm well aware of the behaviour here and have argued it's
the right thing, and wrote `testing::EndsWith` inline, extracted it, and didn't
check the output (as it was offscreen).
@adamcz FYI
If we see this a lot maybe we should consider something like "if all existing
usings in the block are globally qualified...". Current behavior is definitely
defensible though.
================
Comment at: clang/lib/Index/IndexingAction.cpp:142
+ ShouldSkipFunctionBody =
+ [ShouldTraverseDecl(Opts.ShouldTraverseDecl)](const Decl *D) {
+ return !ShouldTraverseDecl(D);
----------------
kadircet wrote:
> are we happy with the copy here ?
>
> I am a little uneasy, as these functions can preserve state. Currently the
> one used in here and the one used in `IndexingContext::indexTopLevelDecl` are
> different copies.
>
> Maybe rather progate a null function into the `IndexASTConsumer` and create
> this default lambda in there, making use of the function inside
> `IndexingContext::IndexOpts` without a copy?
> are we happy with the copy here ?
Note that we already have a copy: we take IndexingOptions by const ref and
createIndexingASTConsumer copies it, including the function. So anything that
relies on the identity of the function passed in, or breaks semantics of
copying, is busted already.
as for internal state (e.g. a cache)... I'd agree if this were a heavily used
library function (find a way to capture by pointer/shared_ptr). But it's not,
and neither of the in-tree callers work this way - I'm not sure it's worth
spending API complexity on.
> Maybe rather progate...
Hmm, I like the idea that these are clearly separate options at the lower
internal layer, and that blurs the lines a bit.
(I'd like *more* making them completely the same, but halfway isn't a happy
place to be I think)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80296/new/
https://reviews.llvm.org/D80296
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
