Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support using better-enum as key in dictionaries and maps. #77

Merged
merged 7 commits into from
Jul 16, 2019
Merged

Support using better-enum as key in dictionaries and maps. #77

merged 7 commits into from
Jul 16, 2019

Conversation

svnscha
Copy link
Contributor

@svnscha svnscha commented Jul 11, 2019

Thanks for better-enum. It is awesome. However I was missing a feature to use the enum values in unordered_map. I have extended it for this use case. Please review and accept my PR.

int main()
{
	Channel channel = Channel::Red;
	
	std::unordered_map<Channel, int> u = {
		{Channel::Red, 1},
		{Channel::Blue, 3},
	};

	printf("Red %d\n", u[Channel::Red]);
	printf("Blue %d\n", u[Channel::Blue]);

	return 0;
}

svnscha added 2 commits July 11, 2019 08:28
This enabled hashing so that unordered_map are supported.
@svnscha
Copy link
Contributor Author

svnscha commented Jul 11, 2019

My previous attempt does not work and I totally did not think about that situation that a better enum could be in a namespace. Then the specialization does fail.

However, how about that approach?

namespace test {

BETTER_ENUM(Channel, char, Red = 1, Green, Blue)

}

BETTER_ENUMS_DECLARE_STD_HASH(test::Channel)

int main()
{
	test::Channel channel = test::Channel::Red;
	
	std::unordered_map<test::Channel, int> u = {
		{test::Channel::Red, 1},
		{test::Channel::Blue, 3},
	};

	printf("Red %d\n", u[test::Channel::Red]);
	printf("Blue %d\n", u[test::Channel::Blue]);

	return 0;
}

enum.h Outdated
@@ -1283,4 +1283,17 @@ BETTER_ENUMS_CONSTEXPR_ map<Enum, T> make_map(T (*f)(Enum))

}

#ifndef BETTER_ENUMS_DECLARE_STD_HASH
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check shouldn't be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, will handle that.

enum.h Outdated
{ \
size_t operator()(const type &x) const \
{ \
return std::hash<size_t>()(x._to_index()); \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use the index rather than the value? _to_index runs a loop internally, whereas _to_integral is just a cast. Is this deliberate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed. I am going to update that to use _to_integral.

@aantron
Copy link
Owner

aantron commented Jul 11, 2019

However, how about that approach?

If it works, I will merge it :)

If possible, can you add a test case that exercises this? It should probably go somewhere in this block:

// Using BETTER_ENUMS_HAVE_CONSTEXPR_ as a proxy for C++11 support. This should
// be changed to be more precise in the future.
#ifdef BETTER_ENUMS_HAVE_CONSTEXPR_

@svnscha
Copy link
Contributor Author

svnscha commented Jul 11, 2019

However, how about that approach?

If it works, I will merge it :)

If possible, can you add a test case that exercises this? It should probably go somewhere in this block:

// Using BETTER_ENUMS_HAVE_CONSTEXPR_ as a proxy for C++11 support. This should
// be changed to be more precise in the future.
#ifdef BETTER_ENUMS_HAVE_CONSTEXPR_

Do you mean by making a test case that tests the unordered_map?

@aantron
Copy link
Owner

aantron commented Jul 11, 2019

Do you mean by making a test case that tests the unordered_map?

Yes.

@svnscha
Copy link
Contributor Author

svnscha commented Jul 12, 2019

I am going to add the test case on Monday.

@svnscha
Copy link
Contributor Author

svnscha commented Jul 15, 2019

I have added a new runtime test @aantron as I dont know if it is really possible to make a std::hash operator usable in a constexpr. Would you chheck it out and see if that is how you wanted it?

@aantron
Copy link
Owner

aantron commented Jul 15, 2019

A run-time test is fine. It looks like it's not compiling, though. See, e.g., https://travis-ci.org/aantron/better-enums/jobs/558780607#L1386.

@svnscha
Copy link
Contributor Author

svnscha commented Jul 16, 2019

Wow, finally.. @aantron ^^

@aantron aantron merged commit 5a677ac into aantron:master Jul 16, 2019
@aantron
Copy link
Owner

aantron commented Jul 16, 2019

:p

Thanks!

aantron added a commit that referenced this pull request Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants