[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clangd/index/CanonicalIncludes.cpp:123
 
   static const std::vector<std::pair<const char *, const char *>>
       SystemHeaderMap = {
----------------
ioeric wrote:
> ioeric wrote:
> > hokein wrote:
> > > sammccall wrote:
> > > > hokein wrote:
> > > > > ioeric wrote:
> > > > > > Can we remove the suffix header mapping now? Is it for the `std::chrono::` symbols? What are the reasons not to include them in this patch? 
> > > > > Ideally, we could remove it once we get a perfect symbol mapping but I'd prefer to keep it (it serves as a fallback when we don't find symbols in the symbol mapping), so that we could add new symbol mapping increasingly without changing the current behavior.
> > > > > 
> > > > > Removing it should be done carefully without introducing regression, and is outside the scope of this patch.
> > > > We do need fallback heuristics. We should conisder cases:
> > > >  - for known `std::` symbols mapped to one system header, we don't need a fallback
> > > >  - for known `std::` mapped to multiple system headers (`std::move`), we need some fallback. There are probably few enough of these that it doesn't justify listing *all* system header paths.
> > > >  - for known `std::` symbols with a primary template in one file and specializations/overloads in another (swap?) always inserting the primary seems fine 
> > > >  - For "random" unknown symbols from system header `foo/bits/_bar.h`, we can not insert, correct to `<bar>`, or use the full path name. I don't have a strong preference here, maybe we should do what's easiest.
> > > >  - for c standard library (`::printf` --> `<stdio.h>` etc) we probably want mappings just like in this patch, I think cppreference has them.
> > > >  - other "standardish" headers (POSIX etc) - hmm, unclear.
> > > > 
> > > > Thinking about all these cases, I'm thinking the nicest solution would be having the symbol -> header mapping, having a small (symbol,header) -> header mapping **only** for ambiguous cases, and not inserting "system" headers that aren't in the main mapping at all. Then we can improve coverage of posix, windows etc headers over time.
> > > > Question is, how can we recognize "system" headers?
> > > I think we were talking about C++ std symbols.
> > > 
> > > > for known std:: symbols mapped to one system header, we don't need a fallback
> > > > for known std:: mapped to multiple system headers (std::move), we need some fallback. There are probably few enough of these that it doesn't justify listing *all* system header paths.
> > > > for known std:: symbols with a primary template in one file and specializations/overloads in another (swap?) always inserting the primary seems fine
> > > 
> > > As discussed in this patch, we have other ways to disambiguate these symbols (rather than relying on the header mapping), so it is possible to remove STL headers mapping, but we'll lose correct headers for STL implement-related symbols (e.g. `__hash_code`), ideally we should drop these symbols in the indexer...
> > > 
> > > > for c standard library (::printf --> <stdio.h> etc) we probably want mappings just like in this patch, I think cppreference has them.
> > > 
> > > Yes, cppreference has them.
> > > 
> > > > other "standardish" headers (POSIX etc) - hmm, unclear.
> > > 
> > > I think we should still keep the header mapping. Not sure whether there are some sources like cppreference that list all symbols.
> > > 
> > > 
> > So, do we have a plan on how to handle ambiguous symbols properly? If so, could you please summarize this in the FIXME comment so that we don't lose track of it?
> > 
> > > I think we should still keep the header mapping. Not sure whether there are some sources like cppreference that list all symbols.
> > What's the reasoning?
> > 
> > I am +1 on Sam's idea about skipping include insertion for std:: symbols that are not on cppreference. It's fine to keep the suffix header mapping in the short term while we are working out ambiguous symbols. But I don't think we would want to maintain both mappings in the long term. 
> > 
> > Could you maybe add comment for the header mapping indicating that they are now the fallback mapping for std:: symbols?
> This is not done?
Oops, I missed it. Added a FIXME.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345