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

Mon Jan 23 09:22:50 PST 2023

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>)
----------------
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.

================
Comment at: clang/include/clang/Tooling/Inclusions/StdSymbolMap.inc:369
+SYMBOL(div, std::, <cinttypes>)
+SYMBOL(div, std::, <cstdlib>)
 SYMBOL(div_t, std::, <cstdlib>)
----------------
hokein wrote:
> this one as well, both headers provide different overloads.
I suggest to special-case the overloads for now, just not to solve all the problems in the same patch.

================
Comment at: clang/tools/include-mapping/cppreference_parser.py:174
-      # std::remove<> has variant algorithm.
-      "std::remove": ("algorithm"),
-  }
----------------
kadircet wrote:
> VitaNuo wrote:
> > kadircet wrote:
> > > this is actually checking for something else (sorry for the confusing naming).
> > > 
> > > the `variant` here refers to library name mentioned in parentheses (this is same problem as `std::move`) on the std symbol index page https://en.cppreference.com/w/cpp/symbol_index (e.g. `remove<>() (algorithm)`). by getting rid of this we're introducing a regression, as previously `std::remove` wouldn't be recognized by the library, but now it'll be recognized and we'll keep suggesting `<cstdio>` for it.
> > > 
> > > so we should actually keep this around.
> > Ok, I can keep this out of this patch, but we'll have to remove this logic evetually when we deal with overloads.
> > 
> > I have a slight suspicion that this code might be buggy, because it suggests that one _of_ the variants should be accepted. What is does in reality, though, is it keeps `algorithm` in the list of headers suitable for `std::remove` alongside `cstdio`, and then in the last step `std::remove` is ignored by the generator because of being defined in two headers.
> > 
> > With this patch, the result will be both `{cstdio, algorithm}`. Is this (more) satisfactory for now compared to skipping `algorithm` due to being an overload?
> > 
> > Ok, I can keep this out of this patch, but we'll have to remove this logic evetually when we deal with overloads.
> 
> Surely, I wasn't saying this should stay here forever, i am just saying that what's done in the scope of this patch doesn't really address the issues "worked around" by this piece.
> 
> > I have a slight suspicion that this code might be buggy, because it suggests that one _of_ the variants should be accepted. What is does in reality, though, is it keeps algorithm in the list of headers suitable for std::remove alongside cstdio, and then in the last step std::remove is ignored by the generator because of being defined in two headers.
> 
> right, it's because we have logic to prefer "non-variant" versions of symbols when available (i.e. in the absence of this logic, we'd prefer std::remove from cstdio). this logic enables us to preserve certain variants (in addition to non-variants). that way we treat std::remove as ambigious rather than always resolving to <cstdio>, hence it's marked as "missing", similar to `std::move`.
> 
> > With this patch, the result will be both {cstdio, algorithm}. Is this (more) satisfactory for now compared to skipping algorithm due to being an overload?
> 
> in the end this should probably look like {algorithm, cstdio}, but as mentioned elsewhere, this is not the same problem as "same symbol being provided by multiple header" but rather "different symbols having same name and different headers". so treatment of it shouldn't change by this patch.
On second thought, I think we'd rather special-case the overloads for now (until we get to them). See the newest patch version.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142092