[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:1053
 SYMBOL(remainder, std::, <cmath>)
+SYMBOL(remove, std::, <cstdio>)
 SYMBOL(remove_all_extents, std::, <type_traits>)
----------------
hokein wrote:
> I think `std::remove` should not be in the scope of the patch, it has two variants:
> - std::remove from `<cstdio>`
> - and std::remove from `<algorithm>`.
Yes, agreed. Let's take care of the overloads going forward.


================
Comment at: clang/tools/include-mapping/cppreference_parser.py:174
-      # std::remove<> has variant algorithm.
-      "std::remove": ("algorithm"),
-  }
----------------
VitaNuo wrote:
> 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.
> 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).

Where is this logic? AFAICS the generator in the current state doesn't generate anything for std::remove. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142092