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

Mon Sep 9 08:27:19 PDT 2019

sammccall added a comment.

In D67172#1663179 <https://reviews.llvm.org/D67172#1663179>, @ilya-biryukov wrote:

> > "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.

In practice, I expect almost all of these names to be C and thus unqualified.

Names are indeed more portable but unlike path mappings we can't just include both when different platforms want different things, or nothing.
e.g. are we willing to claim that any kind of symbol named `::open` should be from `<sys/stat.h>`, even on windows?
Maybe it's OK, and I can't remember if we had specific bad examples in mind, but seems like there are dragons here.

>> 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.

>From a public API standpoint, it seems cleaner to only have these available once addSystemHeadersMapping has been called (with any argument), and delay assigning the pointer until then.
But if adds more than a few lines, it seems fine without.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67172