sammccall added inline comments.

================
Comment at: clangd/index/CanonicalIncludes.cpp:123
 
   static const std::vector<std::pair<const char *, const char *>>
       SystemHeaderMap = {
----------------
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?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to