[PATCH] D142092: [include-mapping] Allow multiple headers for the same symbol. Choose the first header of available ones.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added inline comments.


================
Comment at: clang/include/clang/Tooling/Inclusions/StdSymbolMap.inc:100
 SYMBOL(atoll, std::, <cstdlib>)
+SYMBOL(atomic, std::, <atomic>)
+SYMBOL(atomic, std::, <memory>)
----------------
VitaNuo wrote:
> kadircet wrote:
> > hokein wrote:
> > > Conceptually, this (and other `atomic_*` symbols) doesn't feel correct:
> > > - `<atomic>` provides the generic template `template<class T> struct atomic;`
> > > -  `<memory>` provides partial template specializations for `std::shared_ptr` and `std::weak_ptr`  
> > > 
> > > They are variant symbols (ideally, they should be treat as the `std::move()`). The issue here is that unlike `std::move` which has two individual entries in the index page, we only have one entry for `std::atomic` (extending the cppreference_parser.py script to perfectly distinguish these two cases in the [page](https://en.cppreference.com/w/cpp/atomic/atomic) seems non-trivial).  Some options:
> > > 
> > > 1) treat them as multiple-header symbols (like what this patch does now)
> > > 2) special-case these symbols like `std::move()`
> > > 3) always prefer the header providing generic templates
> > > 
> > > @kadircet, what do you think?
> > right, i believe this patch is definitely adding keeping multiple headers for a symbol around, but mixing it with keeping headers for variants (e.g. overloads provided by different headers, or specializations as mentioned here).
> > 
> > we definitely need some more changes in parser to make sure we're only preserving alternatives for **the same symbol** and not for any other variants (overloads, specializations). IIRC there are currently 2 places these variants could come into play:
> > - first is the symbol index page itself, symbols that have ` (foo)` next to them have variants and should still be ignored (e.g. std::remove variant of cstdio shouldn't be preserved in the scope of this patch)
> > - second is inside the detail pages for symbols, e.g. [std::atomic](http://go/std::atomic), we can have headers for different declarations, they're clearly different variants so we shouldn't add such symbols into the mapping `_ParseSymbolPage` already does some detection of declarations in between headers.
> > 
> > in the scope of this patch, we should keep ignoring both.
> I suggest to special-case the overloads for now, just not to solve all the problems in the same patch.
The first group are already ignored. We need to take a lot at how to ignore the second one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142092