Skip to content

rocksdb_replicator: add a util function ReturnCodeString#608

Open
jaricftw wants to merge 1 commit intomasterfrom
jz/return-code
Open

rocksdb_replicator: add a util function ReturnCodeString#608
jaricftw wants to merge 1 commit intomasterfrom
jz/return-code

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

}
}

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.

}
}

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.

3 participants