sammccall added a comment.
Thanks for putting this together, it definitely seems like something people
want.
My main concern is that the configuration has the wrong scope.
We're checking whether reserved-ident indexing is on for the **TU** we're
indexing, but really we want to specify which **headers** we should index
reserved-idents from.
If hell freezes over and the linux kernel starts using the C++ standard library
one day, we want the preamble index to contain `__free_pages()` but not
`std::__variant_impl_helper`. But the preamble indexing all runs under the same
config.
(Also we more or less assume that all TUs will index a header the same way: if
we've indexed foo.h under A.cpp, we won't reindex it under B.cpp even if the
config is different. So we get "first-one-wins" behavior...)
I don't think we can afford to re-evaluate the config each time we switch
between headers while indexing. (Stupid over-flexible config system is too
slow... I've learned my lesson from that one!)
So I'm not really sure what to suggest...
- we could do it this way anyway and accept that it's basically only usable as
a global flag: this is probably fine for the linux kernel as it's mostly
isolated anyway (no standard library).
- we could add some rules (regexes?) to the config that describe which headers
can have interesting symbols. This could be slow too, but at least cacheable,
like the self-contained check
- maybe we can come up with some heuristics to improve the "no reserved names"
rule, with or without configuration. (Maybe apply it to only -isystem headers?
Linux uses angle-quoted includes, but not actually `-isystem` AFAICS)
What do you think? Is this a real concern or am I missing something?
================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:205
std::optional<Located<bool>> StandardLibrary;
+ /// Whether identifiers reserved for use by the implementation (e.g.
+ /// beginning with two underscores) should be indexed.
----------------
nit: it's the *symbols* we're indexing, based on whether their names are
reserved identifiers.
================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:207
+ /// beginning with two underscores) should be indexed.
+ std::optional<Located<bool>> ReservedIdentifiers;
};
----------------
For some reason I find the name "ReservedNames" a little clearer.
I tend to think of an "identifier" as a type of token at the lexer level, and a
"name" as its role at the language level. But I don't think this actually
matches any standard terminology, so no need to change it unless it also reads
better to you.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153946/new/
https://reviews.llvm.org/D153946
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
