ioeric added inline comments.
================
Comment at: clangd/include-mapping/gen_std.py:46
+//
+// Generated from cppreference offline HTML book v%s.
+//===----------------------------------------------------------------------===//
----------------
I'd suggest dropping `v` prefix as it might be confusing (e.g. one would search
for the exact same version number on cppreference archive). Maybe add a short
comment explaining that this is the modification date of the doc?
================
Comment at: clangd/include-mapping/gen_std.py:114
+ # so we use the modified time of the symbol_index.html as the version.
+ cppreference_version = datetime.datetime.fromtimestamp(
+ os.stat(index_page_path).st_mtime).strftime('%Y-%m-%d')
----------------
s/version/date/, as this not really a version.
================
Comment at: clangd/include-mapping/gen_std.py:129
+ headers = ParseSymbolPage(f.read())
+ if len(headers) == 0:
+ sys.stderr.write("No header found for symbol %s at %s\n" %
(symbol_name,
----------------
`if not headers:`
================
Comment at: clangd/index/CanonicalIncludes.cpp:123
static const std::vector<std::pair<const char *, const char *>>
SystemHeaderMap = {
----------------
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?
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
