[libcxx-commits] [PATCH] D125394: LWG-3657

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added subscribers: ldionne, philnik.
philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.

Could you rename the PR to something along the lines of `[libc++] Implement LWG3657` to make clear which project this is part of and that you are implementing LWG3657?



================
Comment at: libcxx/docs/Status/Cxx17Issues.csv:319-320
 "`2956 <https://wg21.link/LWG2956>`__","``filesystem::canonical()``\  still defined in terms of ``absolute(p, base)``\ ","Toronto","|Complete|",""
+"","","","",""
+"`3657 <https://wg21.link/LWG3657>`__","``std::hash<std::filesystem::path>`` is not enabled","","|Complete|",""
----------------
This is currently in `Cxx2bIssues.csv`. I don't know if it should be there or here. @ldionne
In the last "" should be the version in which it was implemented (15.0) in this case.


================
Comment at: libcxx/include/__filesystem/path.h:1022
 
+_LIBCPP_AVAILABILITY_FILESYSTEM_PUSH
+
----------------
You don't need availability for this function. It's not part of the dylib.


================
Comment at: libcxx/include/__filesystem/path.h:1027
+template <>
+struct _LIBCPP_TEMPLATE_VIS hash<_VSTD_FS ::path> : public unary_function<_VSTD_FS ::path, size_t> {
+
----------------
Please don't inherit from unary_function. This should be removed since C++17.


================
Comment at: libcxx/include/__filesystem/path.h:1029
+
+  _LIBCPP_INLINE_VISIBILITY
+  size_t operator()(const _VSTD_FS ::path& __val) const _NOEXCEPT { return _VSTD_FS ::hash_value(__val); }
----------------
Please use `_LIBCPP_HIDE_FROM_ABI`.


================
Comment at: libcxx/include/__filesystem/path.h:1030
+  _LIBCPP_INLINE_VISIBILITY
+  size_t operator()(const _VSTD_FS ::path& __val) const _NOEXCEPT { return _VSTD_FS ::hash_value(__val); }
+};
----------------



================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.member/path.compare.pass.cpp:12
 // <filesystem>
+// <functional>
 
----------------
This doesn't test <functional>.


================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.member/path.compare.pass.cpp:36
 
 #include "filesystem_include.h"
+#include <functional>
----------------
Please add an empty line here.


================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.nonmember/hash_tested_elswhere.pass.cpp:1
 //===----------------------------------------------------------------------===//
 //
----------------
@ldionne Can we just remove this file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125394