Skip to content

rocksdb_replicator: add a util function ReturnCodeString #608

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.

Sign up for GitHub

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

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

rocksdb_replicator: add a util function ReturnCodeString #608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions rocksdb_replicator/tests/utils_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,20 @@ TEST(ReplicatorUtilsTest, ReplicaRoleString) {
}
}

TEST(ReplicatorUtilsTest, ReturnCodeString) {
std::unordered_map<replicator::ReturnCode, std::string> testscases = {
{replicator::ReturnCode::OK, "OK"},
{replicator::ReturnCode::DB_NOT_FOUND, "DB_NOT_FOUND"},
{replicator::ReturnCode::DB_PRE_EXIST, "DB_PRE_EXIST"},
{replicator::ReturnCode::WRITE_TO_SLAVE, "WRITE_TO_SLAVE"},
{replicator::ReturnCode::WRITE_ERROR, "WRITE_ERROR"},
{replicator::ReturnCode::WAIT_SLAVE_TIMEOUT, "WAIT_SLAVE_TIMEOUT"},
};
for (auto p : testscases) {
EXPECT_EQ(p.second, replicator::ReturnCodeString(p.first));
}
}

int main(int argc, char** argv) {
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
Expand Down
21 changes: 21 additions & 0 deletions rocksdb_replicator/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#pragma once

#include "rocksdb_replicator/thrift/gen-cpp2/Replicator.h"
#include "rocksdb_replicator/rocksdb_replicator.h"

namespace replicator { // namespace replicator

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

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

@kangnanli kangnanli Apr 8, 2022

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 •
edited
Loading

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

@kangnanli kangnanli Apr 8, 2022

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

@mradwan mradwan May 20, 2022

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

@kangnanli kangnanli May 20, 2022

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

@mradwan mradwan May 20, 2022

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

@kangnanli kangnanli May 20, 2022

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

@mradwan mradwan May 20, 2022

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 •
edited
Loading

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.

switch(code) {
case ReturnCode::OK:
return "OK";
case ReturnCode::DB_NOT_FOUND:
return "DB_NOT_FOUND";
case ReturnCode::DB_PRE_EXIST:
return "DB_PRE_EXIST" ;
case ReturnCode::WRITE_TO_SLAVE:
return "WRITE_TO_SLAVE" ;
case ReturnCode::WRITE_ERROR:
return "WRITE_ERROR" ;
case ReturnCode::WAIT_SLAVE_TIMEOUT:
return "WAIT_SLAVE_TIMEOUT" ;

default:
return "__unknown_code__";
}
}

} // namespace replicator