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