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

Fri Jul 21 14:24:26 PDT 2023

sammccall added a comment.

In D153946#4521576 <https://reviews.llvm.org/D153946#4521576>, @nridge wrote:

> Yeah, I'm happy to go with D155381 <https://reviews.llvm.org/D155381> instead.
>
> In D153946#4503585 <https://reviews.llvm.org/D153946#4503585>, @sammccall wrote:
>
>> (Stupid over-flexible config system is too slow... I've learned my lesson from that one!)
>
> The issues with the config system are not entirely obvious to me. If, by chance, you've written some sort of post-mortem / discussion of the shortcomings and how you might design a config system in the future, I'd be interested to read it.

I haven't, but this is a useful idea and I'd like to.

The main regret is making it too powerful/complicated without properly considering the consequences:

- the `if` conditions and the way we plug in fragments mean we really need to compute the config for each file we want to consider. Really, probably few conditions are used and something with simpler fs-hierarchical scope would be much easier to cache.
- to avoid having to do too much interpretation of the config as we repeatedly process the same file, we have the compile step - but this makes processing many *different* files in quick succession more expensive

As a result, we can *barely* use it to control background indexing (evaluating the config to know whether each file in the project should be indexed takes nontrivial time) and we can't use it for things like this where we want to configure per-header but this would end up significantly increasing indexing cost.

There are some other things, like the amount of plumbing that's needed for new settings. It would be good to reflect on the alternatives a bit again, though.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153946