[PATCH] D114724: [clangd][StdSymbolMap] Prefer std::remove from algorithm

[Sam McCall via Phabricator via cfe-commits]

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.
> WDYT?

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):
         continue
----------------
kadircet wrote:
> 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"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114724