[PATCH] D142836: [clangd] Add symbol mappings for `std::experimental::filesystem`

[Kadir Cetinkaya via Phabricator via cfe-commits]

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