VitaNuo added a comment.
Thank you all for comments! The patch should be ready for the next review now.
================
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>)
----------------
VitaNuo wrote:
> 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.
The latest version keeps the `std::remove` overload from `<algorithm>`.
================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:58
+ int SymIndex = NextAvailSymIndex;
+ if (NSSymbolMap *NSSymbols = NamespaceSymbols->lookup(NS)) {
+ auto It = NSSymbols->find(Name);
----------------
hokein wrote:
> VitaNuo wrote:
> > hokein wrote:
> > > Given the fact that multiple-header symbols are grouped in the .inc file,
> > > we could simplify the code of checking a new symbol by looking at the
> > > last available SymIndex:
> > >
> > > ```
> > > if (NextAvailSymIndex > 0 && SymbolNames[NextAvailSymIndex-1].first == NS
> > > && SymbolNames[NextAvailSymIndex-1].second == Name) {
> > > // this is not a new symbol.
> > > }
> > > ```
> > I know this is easier in terms of code, but this heavily relies on the
> > order in the generated files. I would prefer to keep the library and the
> > generator as decoupled as possible, even if it means slightly more complex
> > code here. Overall, it seems more future-proof in case of unrelated
> > generator changes (bugs?) that might change the order.
> > but this heavily relies on the order in the generated files.
>
> Yeah, this is true. I actually think we should make it as an invariant
> (multiple-header symbols are grouped) of the generated .inc files. This
> invariant is important and useful, it is much easier for human to read and
> justify. We can probably guarantee it in the generator side.
Ok, sure. Sorted the symbols in the generator now and also applied the snippet
from above. I'm not 100% sure the code became much simpler but this version
seems fine too :)
================
Comment at: clang/tools/include-mapping/cppreference_parser.py:174
- # std::remove<> has variant algorithm.
- "std::remove": ("algorithm"),
- }
----------------
kadircet wrote:
> VitaNuo wrote:
> > 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.
> > Where is this logic? AFAICS the generator in the current state doesn't
> > generate anything for std::remove.
>
> It's the logic that you're deleting:
> ```
> if variant and variant not in variants_for_symbol:
> continue
> ```
>
> we ignore any symbols that has a variant we shouldn't accept and always
> prefer the non-variant versions. to be more concrete when parsing c++ symbol
> index we'll see two alternatives for `std::remove`:
>
> ```
> remove()
> remove<>() (algorithm)
> ```
>
> first one is a non-variant, hence it's accepted by default. second one is
> `algorithm` variant, and is accepted per this deleted logic. in the end we
> **used** to didn't generate anything because we now have multiple headers
> providing `std::remove`.
>
> i think it creates more confusion to special case only std::remove down
> below, and not other symbols whose non-variant versions we accept, e.g. sin
> also has non-variant versions, but we don't "special case" it to keep it out
> of the map.
>
> i'd suggest treating std::remove similar to other symbols with an accepted
> variant, rather than creating a divergence. so my suggestion is changing the
> logic above to **only** accept variants mentioned in the map when there's a
> mapping for the symbol (and reject the non-variant version). net effect will
> be that we'll now include the mapping from `std::remove` to `<algorithm>`,
> which is currently inline with our behaviour against other symbols with
> different variants. we try to pick the most common variant, and that's almost
> always the "non-variant" version, apart from `std::remove`.
>
> that way we should get rid of the special casing below completely. does that
> make sense?
Ok, I've tweaked the logic to keep `std::remove` from `<algorithm>` now.
================
Comment at: clang/tools/include-mapping/gen_std.py:112
for symbol in symbols:
- if len(symbol.headers) == 1:
- # SYMBOL(unqualified_name, namespace, header)
- print("SYMBOL(%s, %s, %s)" % (symbol.name, symbol.namespace,
- symbol.headers[0]))
- elif len(symbol.headers) == 0:
+ if re.match("^remove$|^swap$", symbol.name) and symbol.namespace ==
"std::":
+ continue
----------------
kadircet wrote:
> different headers providing `std::swap` don't provide different variants of
> it, these are just ADL extension points, similar to std::begin (which also
> shouldn't be missing from the mapping).
>
> for `remove`, i've provided more context in the discussion above, about
> keeping a different variant of `std::remove` around.
Thank you. The latest version should take care both of the overloads that have
accidentally made it to this patch and of the `std::remove` case.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142092/new/
https://reviews.llvm.org/D142092
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
