kadircet added a comment. thanks a lot! mostly LG couple more nits
================ Comment at: clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc:1 +// These symbols are exported from N4100[fs.filesystem.synopsis], the final +// draft for experimental filesystem. Note that not all of these symbols were ---------------- i think comment is a little too verbose. can you just say: ``` These are derived from N4100[fs.filesystem.synopsis], final draft for experimental filesystem. ``` There's no need for mentioning that these became the standard in C++17 or being a cornerstone for stdlib implementations. As they won't necessarily be true for other technical specs nor if we were adding this pre-c++17 standardisation. But we'd still like to have these widely adapted symbols included in the mapping to make sure we're not generating false negatives. ================ Comment at: clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc:6 +// whatever. +// clang-format off +SYMBOL(absolute, std::experimental::filesystem::, <experimental/filesystem>) ---------------- can you strip clang-format pragmas to be similar to other mapping files. ================ Comment at: clang/unittests/Tooling/StandardLibraryTest.cpp:83 +TEST(StdlibTest, Experimental) { + EXPECT_TRUE( + stdlib::Header::named("<experimental/filesystem>", stdlib::Lang::CXX)); ---------------- another EXPECT_FALSE with `Lang::C` would also be important to make sure we're not adding these for C mappings by mistake. ================ Comment at: clang/unittests/Tooling/StandardLibraryTest.cpp:91 + EXPECT_EQ(Symbol->name(), "path"); + EXPECT_EQ(Symbol->qualifiedName(), "std::experimental::filesystem::path"); + ---------------- can you also check for `Symbol->headers()` ================ Comment at: clang/unittests/Tooling/StandardLibraryTest.cpp:93 + + // system_complete is replaced by std::filesystem::absolute + Symbol = stdlib::Symbol::named("std::filesystem::", "system_complete"); ---------------- i don't think there's much point in asserting these "meta" details about the mapping (same for the test below). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142836/new/ https://reviews.llvm.org/D142836 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits