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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
