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

Add enum_reflection.h & preproc.h #10665

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rockeet
Copy link
Contributor

@rockeet rockeet commented Sep 13, 2022

2 years ago, I had created PR #7081 which failed on old MSVC.

Now rocksdb has upgraded to c++17, PR #7081 will not fail in CI, we can continuously migrating existing enum/string conversion code by using this enum refelection.

Below is the brief introduction about enum reflection(copied from PR #7081):


With enum reflection, we can convert enum to/from string

For example:

ROCKSDB_ENUM_PLAIN(CompactionStyle, char,
  kCompactionStyleLevel = 0x0,
  kCompactionStyleUniversal = 0x1,
  kCompactionStyleFIFO = 0x2,
  kCompactionStyleNone = 0x3 // comma(,) can not be present here
);
assert(enum_name(kCompactionStyleUniversal) == "kCompactionStyleUniversal");
assert(enum_name(CompactionStyle(100)).size() == 0);
CompactionStyle cs= kCompactionStyleLevel;
assert(enum_value("kCompactionStyleUniversal", &cs) && cs == kCompactionStyleUniversal);
assert(!enum_value("bad", &cs) && cs == kCompactionStyleUniversal); // cs is not changed

There are 4 macros to define a enum with reflection

// plain old enum defined in a namespace(not in a class/struct)
ROCKSDB_ENUM_PLAIN(EnumType, IntRep, e1 = 1, e2 = 2);
// this generates:
enum EnumType : IntRep { e1 = 1, e2 = 2 };
// enum reflection supporting code ...
// ...
// the supporting code makes template function enum_name and
// enum_value works for this EnumType

// other three macros are similar with some difference:

// enum class defined in a namespace(not in a class/struct)
ROCKSDB_ENUM_CLASS(EnumType, IntRep, Enum values ...);

// plain old enum defined in a class/struct(not in a namespace)
ROCKSDB_ENUM_PLAIN_INCLASS(EnumType, IntRep, Enum values ...);

// enum class defined in a class/struct (not in a a namespace)
ROCKSDB_ENUM_CLASS_INCLASS(EnumType, IntRep, Enum values ...);

Copy link
Contributor

@mrambacher mrambacher left a comment

Choose a reason for hiding this comment

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

I fully support the need for this reflection. The OptionType has code which would use this for Parse/SerializeEnum already which relies on user-defined maps.

Having said that, I worry that this is a bit confusing and hard to push into the consumer-facing (include/rocksdb) API. Could we have some simple methods added to the public API (like the to/from string), perhaps through some macro and have the implementation of those methods be in some internal code path (util for example). That internal representation could be based on something like this package or some other that is available (like magic_enum.hpp).

My concern is not so much in the functionality being exposed -- I think these methods are highly valuable -- but more as to what ends up in the public API and how. Once enum_reflection etc is in include/rocksdb, it is much harder to change and folks could use it for purposes not intended.

Finally, I think it would be helpful to introduce and use this functionality in a single PR. Would it be possible to introduce the new methods and use them to replace the ParseEnum code (for example)? This would make it easier to validate and understand that the code is "doing the right thing" rather than code without any tests or usage.

};

template<class Enum>
Slice enum_name(Enum v, const char* unkown = "") {
Copy link
Contributor

Choose a reason for hiding this comment

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

"Unknown"

if (v == values[i])
return names.first[i].ToString();
}
return "unkown:" + (sizeof(Enum) <= sizeof(int)
Copy link
Contributor

Choose a reason for hiding this comment

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

"Unknown"

}

template<class Enum>
bool enum_value(const ROCKSDB_NAMESPACE::Slice& name, Enum* result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why "Slice" and not std::string" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

std::string and const char* can be implicitly converted to Slice, and Slice is a fundamental type in rocksdb.

}

template<class Enum>
const char* enum_cstr(Enum v, const char* unkown = "") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not NULL as default?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants