VitaNuo added inline comments.
================ Comment at: clang/include/clang/Tooling/Inclusions/StdSymbolMap.inc:100 SYMBOL(atoll, std::, <cstdlib>) +SYMBOL(atomic, std::, <atomic>) +SYMBOL(atomic, std::, <memory>) ---------------- VitaNuo wrote: > kadircet wrote: > > hokein wrote: > > > Conceptually, this (and other `atomic_*` symbols) doesn't feel correct: > > > - `<atomic>` provides the generic template `template<class T> struct > > > atomic;` > > > - `<memory>` provides partial template specializations for > > > `std::shared_ptr` and `std::weak_ptr` > > > > > > They are variant symbols (ideally, they should be treat as the > > > `std::move()`). The issue here is that unlike `std::move` which has two > > > individual entries in the index page, we only have one entry for > > > `std::atomic` (extending the cppreference_parser.py script to perfectly > > > distinguish these two cases in the > > > [page](https://en.cppreference.com/w/cpp/atomic/atomic) seems > > > non-trivial). Some options: > > > > > > 1) treat them as multiple-header symbols (like what this patch does now) > > > 2) special-case these symbols like `std::move()` > > > 3) always prefer the header providing generic templates > > > > > > @kadircet, what do you think? > > right, i believe this patch is definitely adding keeping multiple headers > > for a symbol around, but mixing it with keeping headers for variants (e.g. > > overloads provided by different headers, or specializations as mentioned > > here). > > > > we definitely need some more changes in parser to make sure we're only > > preserving alternatives for **the same symbol** and not for any other > > variants (overloads, specializations). IIRC there are currently 2 places > > these variants could come into play: > > - first is the symbol index page itself, symbols that have ` (foo)` next to > > them have variants and should still be ignored (e.g. std::remove variant of > > cstdio shouldn't be preserved in the scope of this patch) > > - second is inside the detail pages for symbols, e.g. > > [std::atomic](http://go/std::atomic), we can have headers for different > > declarations, they're clearly different variants so we shouldn't add such > > symbols into the mapping `_ParseSymbolPage` already does some detection of > > declarations in between headers. > > > > in the scope of this patch, we should keep ignoring both. > I suggest to special-case the overloads for now, just not to solve all the > problems in the same patch. The first group are already ignored. We need to take a lot at how to ignore the second one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142092/new/ https://reviews.llvm.org/D142092 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
