[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

Mon Sep 9 05:30:20 PDT 2019

ilya-biryukov added a comment.

In D67172#1662945 <https://reviews.llvm.org/D67172#1662945>, @sammccall wrote:

> Only if it's 5% of something meaningful. If the denominator isn't something we care about, then it's really "spending XXX usec is not ok" - which depends on XXX I think.

Agree, but unit-test time is meaningful clangd developers. Not saying this is more important than real use-cases, but keeping the unit tests enables us to iterate faster.

> One way this could be simpler is if suffix mappings were gone, then the `StdIncludes` class would go away and we'd just be left with a `stringmap<string>`. Then there'd be a performance win with almost no extra complexity.
>  The description says you're waiting for it to go away, but AFAIK nobody's working on this nor really knows how to eliminate it.

We seem to know how to eliminate this in principle. See @hokein's comment, we could try parsing man pages or even the headers themselves and building a corresponding symbol map.
But this patch does not add suffix mapping, so I don't think that's relevant - this patch tries to avoid redundant initialization; not remove suffix maps.

> Some of the new complexity seems unneccesary even with this approach.
>  There's a lot of indirection around the initialization, an enum that gets passed around a bunch, constructors that switch over it.
>  I think this could be:

Will update the patch. Although I find the current approach more direct, I will happily change the code according to your suggestion.

>>> I'm nervous about requiring langopts, it's convenient in tests/prototypes/etc to be able to default-construct CanonicalIncludes (e.g. a test that doesn't actually care about standard library mappings).
>> 
>> We can have a factory method that constructs with default lang opts for tests, I don't think this would be an issue in the actual application code. In fact, I think it'll make the application code better.
> 
> I'm not saying that there's no way to work around it, or that it's fatal, but my opinion is that this would be worse than the current state.

If we care about having a default constructor, we can default to using `StdIncludes` with a default mapping. I'll avoid requiring `LangOptions` in the constructor in that case. Thanks for the early feedback!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67172