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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
