Page MenuHomePhabricator

Add std::hash<char8_t> specialization if char8_t is enabled
ClosedPublic

Authored by georgthegreat on Nov 30 2020, 7:45 AM.

Details

Summary

Use _LIBCPP_NO_HAS_CHAR8_T to test if template should be specialized.

Diff Detail

Event Timeline

georgthegreat created this revision.Nov 30 2020, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2020, 7:45 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
georgthegreat requested review of this revision.Nov 30 2020, 7:45 AM
mclow.lists added inline comments.Nov 30 2020, 8:12 AM
libcxx/include/utility
1367

I think this should be #ifndef _LIBCPP_NO_HAS_CHAR8_T

libcxx/test/libcxx/extensions/hash/specializations.pass.cpp
26 ↗(On Diff #308368)

Here too.

georgthegreat added a comment.EditedNov 30 2020, 8:19 AM

There is a chance that _LIBCPP_NO_HAS_CHAR8_T should be removed in the course of https://reviews.llvm.org/D92212

Could you please, take a look and share you thoughts?

Commented there.
For the record, the link is https://reviews.llvm.org/D92212 (without the trailing slash.
Alternately, you can just write D92212 and Phab will figure it out.

Use libc++-specific macro to rest if hashing should be enabled.

Continue using standard defined macro in tests.

georgthegreat added inline comments.Dec 1 2020, 2:13 AM
libcxx/test/libcxx/extensions/hash/specializations.pass.cpp
26 ↗(On Diff #308368)

I think test (due to being a library client) should continue checking the flag defined by the standard __cpp_lib_char8_t.

I have fixed it to check the value of the macro,

georgthegreat planned changes to this revision.Dec 1 2020, 12:49 PM

I am waiting for D92212 to land, then I will rebase the diff to make it consistent with the changes proposed by @ldionne.

mclow.lists added inline comments.Dec 2 2020, 9:17 AM
libcxx/test/libcxx/extensions/hash/specializations.pass.cpp
26 ↗(On Diff #308368)

Since the test is libc++-specific (it's in the test/libcxx hierarchy, as opposed to test/std), it *could* use _LIBCPP_NO_HAS_CHAR8_T. However, this is fine.

ldionne accepted this revision.Dec 3 2020, 9:39 AM

We can still proceed with this even though we're keeping _LIBCPP_NO_HAS_CHAR8_T.

This LGTM.

ldionne set the repository for this revision to rG LLVM Github Monorepo.Dec 3 2020, 9:40 AM

@georgthegreat Can you please rebase and resubmit, that will trigger CI.

georgthegreat edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Dec 3 2020, 11:30 AM
georgthegreat marked 3 inline comments as done.Dec 3 2020, 11:30 AM
ldionne accepted this revision.Dec 3 2020, 11:31 AM

Let's wait for CI to complete, and if it pass, you're good to commit this. Thanks!

The attempt to test std::hash via _gnu_cxx was just wrong.
I can not find any place to put individual hash tests anywhere, so I am removing the test completely.

The final revision is buildable and can be merged.
@ldionne, could you merge it?