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

[Younan Zhang via Phabricator via cfe-commits]

zyounan added inline comments.


================
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
----------------
kadircet wrote:
> 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.
Well, I'm not quite sure if I should address these small differences between TS and C++17. But FWIW, the verbosity should go away.


================
Comment at: clang/unittests/Tooling/StandardLibraryTest.cpp:93
+
+  // system_complete is replaced by std::filesystem::absolute
+  Symbol = stdlib::Symbol::named("std::filesystem::", "system_complete");
----------------
kadircet wrote:
> i don't think there's much point in asserting these "meta" details about the mapping (same for the test below).
They're used to emphasize the slight difference. They can be removed if you don't think it necessary :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142836/new/

https://reviews.llvm.org/D142836