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

rocksdb_replicator: add a util function ReturnCodeString #608

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jaricftw
Copy link
Contributor

@jaricftw jaricftw commented Apr 8, 2022

This will be used on the caller side to log/print the return code as string

@@ -32,4 +33,24 @@ const char* ReplicaRoleString(ReplicaRole role) {
}
}

const char* ReturnCodeString(ReturnCode code) {
Copy link
Contributor

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;
  }
};

Copy link
Contributor Author

@jaricftw jaricftw Apr 8, 2022

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?

Copy link
Contributor

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

Copy link
Contributor

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..

Copy link
Contributor

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"?

Copy link
Contributor

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;
}

Copy link
Contributor

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....

Copy link
Contributor

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

Copy link
Contributor

@mradwan mradwan May 21, 2022

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) {
Copy link
Contributor

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..

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.

None yet

3 participants