[PATCH] D153946: [clangd] Add a flag to allow indexing of reserved identifiers

[Sam McCall via Phabricator via cfe-commits]

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