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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/StdSymbolMap.inc:958
 SYMBOL(remainder, std::, <cmath>)
-SYMBOL(remove, std::, <cstdio>)
 SYMBOL(remove_all_extents, std::, <type_traits>)
----------------
sammccall wrote:
> Is the stdio version so uncommon we are willing to be wrong about it? For `move` we omit it from here and consider it case-by-case, I'd have imagined doing the same here 
> 
> (BTW at some point we may want to extend this to list alternative headers for ambiguous symbols)
>Is the stdio version so uncommon we are willing to be wrong about it?

Waiting for permissions on a big codebase with lots of third_party code to perform some analysis.

>  For move we omit it from here and consider it case-by-case, I'd have imagined doing the same here

`std::move` is different, both occurrences are marked as variants. We don't really drop it during post-processing saying there are multiple options, it just never makes it to the final list.

```
move<>() (algorithm) (since C++11)
move<>() (utility) (since C++11)
```

I am not sure how we ended up with this ignore the `variant`ed symbols behavior.

> (BTW at some point we may want to extend this to list alternative headers for ambiguous symbols)

I agree, there is some room for improvement, like also listing different overloads with expected number of parameters and language versions.
It's unclear whether we need such detail today though.


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114724