kadircet 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>) ---------------- hokein wrote: > VitaNuo wrote: > > 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. > Ideally, we should tweak the `_ParseSymbolPage` to handle this case, but I > don't see a easy way to do it (the `atomic` > [case](https://en.cppreference.com/w/cpp/atomic/atomic) is quite tricky where > the symbol name is identical, only the template argument is different, and > the simple text-match heuristic in `_ParseSymbolPage` fails in this case). > > so specialcasing these (there are not too many of them) looks fine to me. I don't follow why we can't perform this detection directly, haven't been taking a look at the details but the page looks like: ``` Defined in header <atomic> template< class T > struct atomic; template< class U > struct atomic<U*>; Defined in header <memory> template< class U > struct atomic<std::shared_ptr<U>>; template< class U > struct atomic<std::weak_ptr<U>>; Defined in header <stdatomic.h> #define _Atomic(T) /* see below */ ``` So `_ParseSymbolPage`, first sees <atomic> then a bunch of decls, then <memory> then a bunch more decls and so on, and in the end it returns all the headers it has found. Why can't we change the logic here to return `nothing` when there are different decls && headers? i.e. return empty if we see a new header after seeing some declarations. does this result in undesired behaviour elsewhere? 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
