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>) ---------------- 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. ================ Comment at: clang/include/clang/Tooling/Inclusions/StdSymbolMap.inc:369 +SYMBOL(div, std::, <cinttypes>) +SYMBOL(div, std::, <cstdlib>) SYMBOL(div_t, std::, <cstdlib>) ---------------- hokein wrote: > this one as well, both headers provide different overloads. I suggest to special-case the overloads for now, just not to solve all the problems in the same patch. ================ Comment at: clang/tools/include-mapping/cppreference_parser.py:174 - # std::remove<> has variant algorithm. - "std::remove": ("algorithm"), - } ---------------- kadircet wrote: > VitaNuo wrote: > > kadircet wrote: > > > this is actually checking for something else (sorry for the confusing > > > naming). > > > > > > the `variant` here refers to library name mentioned in parentheses (this > > > is same problem as `std::move`) on the std symbol index page > > > https://en.cppreference.com/w/cpp/symbol_index (e.g. `remove<>() > > > (algorithm)`). by getting rid of this we're introducing a regression, as > > > previously `std::remove` wouldn't be recognized by the library, but now > > > it'll be recognized and we'll keep suggesting `<cstdio>` for it. > > > > > > so we should actually keep this around. > > Ok, I can keep this out of this patch, but we'll have to remove this logic > > evetually when we deal with overloads. > > > > I have a slight suspicion that this code might be buggy, because it > > suggests that one _of_ the variants should be accepted. What is does in > > reality, though, is it keeps `algorithm` in the list of headers suitable > > for `std::remove` alongside `cstdio`, and then in the last step > > `std::remove` is ignored by the generator because of being defined in two > > headers. > > > > With this patch, the result will be both `{cstdio, algorithm}`. Is this > > (more) satisfactory for now compared to skipping `algorithm` due to being > > an overload? > > > > Ok, I can keep this out of this patch, but we'll have to remove this logic > > evetually when we deal with overloads. > > Surely, I wasn't saying this should stay here forever, i am just saying that > what's done in the scope of this patch doesn't really address the issues > "worked around" by this piece. > > > I have a slight suspicion that this code might be buggy, because it > > suggests that one _of_ the variants should be accepted. What is does in > > reality, though, is it keeps algorithm in the list of headers suitable for > > std::remove alongside cstdio, and then in the last step std::remove is > > ignored by the generator because of being defined in two headers. > > right, it's because we have logic to prefer "non-variant" versions of symbols > when available (i.e. in the absence of this logic, we'd prefer std::remove > from cstdio). this logic enables us to preserve certain variants (in addition > to non-variants). that way we treat std::remove as ambigious rather than > always resolving to <cstdio>, hence it's marked as "missing", similar to > `std::move`. > > > With this patch, the result will be both {cstdio, algorithm}. Is this > > (more) satisfactory for now compared to skipping algorithm due to being an > > overload? > > in the end this should probably look like {algorithm, cstdio}, but as > mentioned elsewhere, this is not the same problem as "same symbol being > provided by multiple header" but rather "different symbols having same name > and different headers". so treatment of it shouldn't change by this patch. On second thought, I think we'd rather special-case the overloads for now (until we get to them). See the newest patch version. 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
