-
Notifications
You must be signed in to change notification settings - Fork 118
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
rocksdb_replicator: add a util function ReturnCodeString #608
base: master
Are you sure you want to change the base?
Conversation
@@ -32,4 +33,24 @@ const char* ReplicaRoleString(ReplicaRole role) { | |||
} | |||
} | |||
|
|||
const char* ReturnCodeString(ReturnCode code) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In thrift, they generate a map with following code in Thrift.h
template <typename EnumTypeT>
struct TEnumMapFactory {
using EnumType = EnumTypeT;
using ValuesToNamesMapType = std::map<EnumType, const char*>;
using NamesToValuesMapType = std::map<const char*, EnumType, ltstr>;
using Traits = TEnumTraits<EnumType>;
static ValuesToNamesMapType makeValuesToNamesMap() {
ValuesToNamesMapType _return;
for (size_t i = 0; i < Traits::size; ++i) {
_return.emplace(EnumType(Traits::values[i]), Traits::names[i].data());
}
return _return;
}
static NamesToValuesMapType makeNamesToValuesMap() {
NamesToValuesMapType _return;
for (size_t i = 0; i < Traits::size; ++i) {
_return.emplace(Traits::names[i].data(), EnumType(Traits::values[i]));
}
return _return;
}
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I looked at many other possible implementations for converting enums to strings: https://stackoverflow.com/questions/207976/how-to-easily-map-c-enums-to-strings, or https://stackoverflow.com/questions/28828957/enum-to-string-in-modern-c11-c14-c17-and-future-c20 but I don't think any of them is simple & performant enough. Ideally what I want is a tool to auto-generate the code that does the translation so we don't have to write any code. We don't want any runtime reflection for perf reasons. Do you have suggestion for one that's available for C++? (e.g. in Go this would be very simple: https://pkg.go.dev/golang.org/x/tools/cmd/stringer)
I am not sure how the above code is used in thrift, is that just something they use to auto-gen an enum to string map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for generation part, I don't know how thrift get it to work, will have to dive into thrift code to get it.
but, for the data structure part, do you think we should consider store it a map? It will be easier to visualize and sort of consistent to thrift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am okay with this implementation, but the way I would go about implementing this is using a bit of meta programming where you would have a typetrait for each enum value.
I did a bit of exploration to see if we can do compile time checks but unfortunately all of them would rely on knowing the enum end marker.
another thing you could think about is using macros, these will probably work but they are not the most fun thing to read..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mradwan could you elaborate on "have a typetrait for each enum value"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kangnanli something like this, I am bit rusty on my templates so this stuff can be improved for sure.
Note: THIS REALLY DOES RECURSION TO lookup, unless compiler optimizes it. however it can be improved for sure by doing binary search in the templated class, I think this might be a viable solution actually
#include <iostream>
enum TestEnum {
A = 0,
B = 1,
END_MARKER
};
template<size_t X>
struct NameTrait;
template<>
struct NameTrait<0> {
static constexpr const char* value = "A_name";
};
template<>
struct NameTrait<1> {
static constexpr const char* value = "B_name";
};
template<size_t CURRENT>
struct Lookup {
static std::string find(size_t key) {
if(key == CURRENT) {
return NameTrait<CURRENT>::value;
}
return Lookup<CURRENT+1>::find(key);
}
};
template<>
struct Lookup<TestEnum::END_MARKER> {
static std::string find(size_t key) {
return "___not__found__";
}
};
int main() {
TestEnum test = TestEnum::A;
std::cout << Lookup<0>::find(test) << std::endl;
return 0;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I though we only need NameTrait<CURRENT>::value
, not for the Lookup
.
overall, this seem to be over complicated....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thing is NameTrait means X needs to be known at compile time.
However size_t key or TestEnum test = TestEnum::A; is a variable in memory thus only known at run time.
So what this code does it will generate code for different structs
Lookup<0>, Lookup<1>, Lookup<2> and then choosing which one to return NameTrait from happen during runtime based on the value in memory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kangnanli I spent sometime thinking how to improve it, and I think the way to go would be extracting everything into library function that can be reused for different enum types thus complexity will be there once.
I did use C++20 tho, because it makes error message much better
Here is how you would implement a good enum
enum TestEnum {
A,
B,
END_MARKER,
};
template<size_t X>
struct NameTrait;
template<>
struct NameTrait<0> {
static constexpr const char* value = "A";
};
template<>
struct NameTrait<1> {
static constexpr const char* value = "B";
};
Here is how the library side looks like
namespace lib {
template<typename T>
concept EndMarkedEnum = requires(T a) {
{ T::END_MARKER } -> std::convertible_to<std::size_t>;
};
template<EndMarkedEnum T, typename C>
struct Lookup;
template<EndMarkedEnum T, typename C>
struct Lookup {
static std::string find(size_t key) {
if(key == C::value) {
return NameTrait<C::value>::value;
}
return Lookup<T, std::integral_constant<size_t, C::value + 1>>::find(key);
}
};
template<EndMarkedEnum T>
struct Lookup<T, std::integral_constant<size_t, T::END_MARKER>> {
static std::string find(size_t key) {
return "NOT_FOUND";
}
};
template<EndMarkedEnum T>
std::string EnumToString(T value){
return Lookup<T, std::integral_constant<size_t, 0>>::find(value);
}
}
Invoking the library will be as simple as this, so no type parameters needed at all
TestEnum test = TestEnum::A;
std::cout << lib::EnumToString(test) << std::endl;
Finally here is how a bad enum will look like
enum BadTestEnum {
Alpha,
Beta,
};
Upon compilation, the enum will output this error message if its used
enums.cpp:63:18: error: no matching function for call to 'EnumToString'
std::cout << lib::EnumToString(test2) << std::endl;
^~~~~~~~~~~~~~~~~
enums.cpp:55:13: note: candidate template ignored: constraints not satisfied [with T = BadTestEnum]
std::string EnumToString(T value){
^
enums.cpp:54:10: note: because 'BadTestEnum' does not satisfy 'EndMarkedEnum'
template<EndMarkedEnum T>
^
enums.cpp:30:10: note: because 'T::END_MARKER' would be invalid: no member named 'END_MARKER' in 'BadTestEnum'
{ T::END_MARKER } -> std::convertible_to<std::size_t>;
^
1 error generated.
Of course all this stuff is possible with std::enable_if however it will look much uglier.
@@ -32,4 +33,24 @@ const char* ReplicaRoleString(ReplicaRole role) { | |||
} | |||
} | |||
|
|||
const char* ReturnCodeString(ReturnCode code) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am okay with this implementation, but the way I would go about implementing this is using a bit of meta programming where you would have a typetrait for each enum value.
I did a bit of exploration to see if we can do compile time checks but unfortunately all of them would rely on knowing the enum end marker.
another thing you could think about is using macros, these will probably work but they are not the most fun thing to read..
This will be used on the caller side to log/print the return code as string