[PATCH] D114724: [clangd][StdSymbolMap] Prefer std::remove from algorithm
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 1 06:26:58 PST 2021
sammccall added a comment.
In D114724#3163579 <https://reviews.llvm.org/D114724#3163579>, @kadircet wrote:
> for whatever reason I remembered the std::remove to have been referenced ~17k. looks like these two are much closer. I don't think it makes sense to prefer one or the other here.
Agree, for StdSymbolMap.inc this means neither version of remove should be there.
> I suggest moving forward with a mechanism that enables accepting certain variants and accepting both unnamed and algorithm variants for std::remove, hence it being rejected as an ambiguous symbol in the end. That way we won't have a special case to handle once we start including multiple options for a symbol.
In practice we don't actually need the ability to accept specific variants, just variants in general.
(Maybe we'll need it later, but maybe not.)
Comment at: clang-tools-extra/clangd/CSymbolMap.inc:701
SYMBOL(remainderl, None, <math.h>)
-SYMBOL(remove, None, <stdio.h>)
SYMBOL(remquo, None, <math.h>)
hmm, I would expect this to still be in CSymbolMap, it's not ambiguous there.
Comment at: clang-tools-extra/clangd/include-mapping/cppreference_parser.py:138
# FIXME: use these as a fallback rather than ignoring entirely.
- if variant:
+ if variant != (namespace+symbol_name in variants_to_accept):
> sammccall wrote:
> > This logic seems a bit weird and non-general: it allows us to accept any variant and reject any non-variant, but it doesn't allow us to accept a*specific* variant (they are named) and doesn't allow us to accept both (which currently would lead to the symbol being dropped as ambiguous).
> I was trying to keep the change to a minimum. Happy to go with a solution that accepts a dictionary in the form of `(FQN, list_of_accepted_variants)`, where the list can have `""` variant, or instead of a list we can also have a glob, but I don't think it'll provide much value considering the irregularity of std header names.
I'd just change this to `variant or (ns + name in variants to accept)`
That way we accept both versions of move, and end up dropping it as ambiguous.
Comment at: clang-tools-extra/clangd/include-mapping/cppreference_parser.py:164
+ # those symbols.
+ variants_to_accept = set(["std::remove"])
symbols = 
we should have a comment on the symbol
# std::remove<> template has variant "algorithm"
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits