[PATCH] D143640: [Tooling/Inclusion] Support overload symbols in the stdlib.

Mon Feb 13 00:37:25 PST 2023

kadircet added a comment.

All this complexity for handling only 4 symbols feels wrong. is this the whole set? are there any other symbols that fall under this class? can we disambiguate all of them solely based on number of parameters?
Downsides:

- We are relying heavily on number of parameters in certain cases now
- We only have that information available for symbols with variants
- We're creating a discrepancy in Symbol::all() vs Symbol::named() views of the world
- In addition to generator and hand curated list, now there's a 3rd place to augment symbol mapping

This complexity both in terms of mental load when introducing new symbols, and also possible discrepancies that can be encountered when using the public APIs makes me question if we're doing the sensible thing that'll generalize for the future variant symbols, or just trying to patch up a solution for couple cases we know to be not working.
I'd suggest:
1- doing a little bit more design work here and at least getting rid of the burden on the public APIs and a way to introduce these symbols through hand curated lists (e.g. we should be returning all alternatives in Symbol::named, with enough details for the caller to have a chance to disambiguate and have maybe new syntax in raw symbol map files to indicate this variant situation).
2- or handling this in include-cleaner only, similar to how we do it in clangd and other applications, until we encounter more problematic symbols. as handling this in the general case seems hard (especially in the absence of an AST), and we haven't seen enough signals for symbols other than `std::move` to cause trouble.

i feel like 2nd alternative gets us a long way with minimal effort. WDYT?

================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:131
                            [&QName](const SymbolHeaderMapping::SymbolName &S) {
                              return S.qualifiedName() == QName;
                            }) &&
----------------
i think we'd now trigger this assert if a variant is provided by multiple headers

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143640