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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
