Page MenuHomePhabricator

[libc++] Implement LWG3657 (std::hash<std::filesystem::path> is not enabled)
Needs ReviewPublic

Authored by Fulgen on May 11 2022, 8:36 AM.

Details

Reviewers
philnik
Group Reviewers
Restricted Project
Summary

This patch implements LWG-3657 (std::hash<std::filesystem::path> is not enabled) in libcxx. Since the February 2022 was virtual, I don't know what location to specify in docs/Status.

Diff Detail

Event Timeline

Fulgen created this revision.May 11 2022, 8:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 8:36 AM
Fulgen requested review of this revision.May 11 2022, 8:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 8:36 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik requested changes to this revision.May 11 2022, 8:53 AM
philnik added subscribers: ldionne, philnik.

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?

libcxx/docs/Status/Cxx17Issues.csv
319–320 ↗(On Diff #428679)

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.

libcxx/include/__filesystem/path.h
1022

You don't need availability for this function. It's not part of the dylib.

1027

Please don't inherit from unary_function. This should be removed since C++17.

1029

Please use _LIBCPP_HIDE_FROM_ABI.

1030
libcxx/test/std/input.output/filesystems/class.path/path.member/path.compare.pass.cpp
12

This doesn't test <functional>.

34–36

Please add an empty line here.

libcxx/test/std/input.output/filesystems/class.path/path.nonmember/hash_tested_elswhere.pass.cpp
1

@ldionne Can we just remove this file?

This revision now requires changes to proceed.May 11 2022, 8:53 AM
Fulgen retitled this revision from LWG-3657 to [libc++] Implement LWG3657.May 11 2022, 9:58 AM
Mordante added inline comments.
libcxx/docs/Status/Cxx17Issues.csv
319–320 ↗(On Diff #428679)

In general LWG issues are listed in the status list C++ version it was voted in the WP.
So if this was voted in the February 2202 virtual plenary it should be in Cxx2bIssues
We still retroactively apply the fix to older versions of the Standard.

Fulgen updated this revision to Diff 428749.May 11 2022, 12:57 PM

Fixes according to review feedback

@Fulgen Fly-by comment: can you please change the title (i.e., the first line) of the patch to include the name of the paper as well? It would make it a little easier to understand what the patch is about for somebody looking through the list of Phabricator reviews (or the output of git log --oneline).

Fulgen retitled this revision from [libc++] Implement LWG3657 to [libc++] Implement LWG3657 (std::hash<std::filesystem::path> is not enabled).May 16 2022, 7:05 AM

Did that now, assuming you meant retitling the revision (there are now three commits in this revision due to feedback fixes, so I don't think you mean the commit message).

It looks like you forgot to add the first commit to you diff.

Fulgen updated this revision to Diff 430043.May 17 2022, 6:57 AM

Add missing commit

Fulgen updated this revision to Diff 430045.May 17 2022, 6:59 AM
  • Readd accidentally deleted newline at end of file

Messed up updating the revision with arc, fixed now.