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

Mon Sep 9 08:03:13 PDT 2019

ilya-biryukov added a comment.

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

> LG, though if we can drop the struct and make MaxSuffixComponents a constant it'd be simpler still.

Done.

> Sure. This is going to win a couple of percent I guess: for these cases we care about walltime including rebuild and lit startup overhead. It's worth having (and thanks for doing this!) but I don't think we should pay much complexity.

Agreed.

> "we could try" sounds like we *don't* know how to eliminate it. Parsing manpages aside, I thought the main problem was these symbols are nonstandard and an infinitely portable qname->header mapping simply didn't exist.

I would expect qualified names to be more portable than paths, but might be mistaken. I recall that we might run into problems if folks define some names as macros instead of functions, but I would be interested to see how common is this.
We should probably write down the examples that are broken somewhere... It's hard to see why it doesn't work without having the concrete cases.
@hokein, do you have any examples that cannot be covered by qual-name-to-path mapping?

> Thanks, this looks a lot better!
>  I think it can be simplified a little further, as the suffix maps are totally hardcoded now.

I think the simplest option is to always make the path suffix mapping available in the class.
That means we will always map the system paths to their shorter versions. It looks ok to me, but might make unit-testing a little more awkward.
WDYT?
I'll land this and send this out as a separate patch if everyone is happy with the approach.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67172