sammccall added inline comments.
================ Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:56 +// C++ and C Standard Library symbols are considered distinct: e.g. std::printf +// and ::printf are not treated as the same symbol. The callers have to act +// accordingly based on available LanguageOptions. ---------------- I'm not sure what "the callers have to act accordingly..." means - can you drop it or elaborate? ================ Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:37 + static llvm::Optional<Symbol> named(llvm::StringRef Scope, + llvm::StringRef Name); + ---------------- kbobyrev wrote: > kadircet wrote: > > should scope have trailing `::` ? > This is consistent with the current behavior; we can probably change it later. Please document the behavior that's being added here. (In clangd, "scope" fairly consistently includes "::" - I don't think that convention exists here) ================ Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:66 + Recognizer(); + llvm::Optional<Symbol> operator()(const Decl *D); + ---------------- kbobyrev wrote: > kadircet wrote: > > what about macros? > For now, I'm just moving the code without adding any new capabilities. The > only change is in `namespaceSymbols` (to break the dependency on clangd > helpers) that Sam pointed out. Please add a mention of macros to the docs. ================ Comment at: clang/lib/Tooling/Inclusions/StandardLibrary.cpp:119 + return namespaceSymbols(Parent); + return NamespaceSymbols->lookup(D->getQualifiedNameAsString() + "::"); + }(); ---------------- Does this do the right thing for `std::__1::chrono` where `__1` is inline? ================ Comment at: clang/unittests/Tooling/StandardLibraryTest.cpp:61 + + namespace chrono {} + ---------------- should there be symbols in this NS and a test for them? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119130/new/ https://reviews.llvm.org/D119130 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits