Page MenuHomePhabricator

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

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



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

I think this should be #ifndef _LIBCPP_NO_HAS_CHAR8_T

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

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

Commented there.
For the record, the link is (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
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
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?