Skip to content

ARROW-13296: [C++] Provide a reflection compatible enum replacement#10691

Closed
bkietz wants to merge 13 commits into
apache:masterfrom
bkietz:13296-Provide-reflection-compat
Closed

ARROW-13296: [C++] Provide a reflection compatible enum replacement#10691
bkietz wants to merge 13 commits into
apache:masterfrom
bkietz:13296-Provide-reflection-compat

Conversation

@bkietz

@bkietz bkietz commented Jul 9, 2021

Copy link
Copy Markdown
Member

Provides an enum replacement with minimal reflection capabilities. These can be declared by inheriting from a helper with CRTP, including a static string literal data member containing the enum's values:

struct Color : EnumType<Color> {
  using EnumType::EnumType;
  static constexpr char* kValues = "red green blue";
};

Values of enumerations declared in this way can be constructed from their string representations at compile time, and can be converted to their string representation for easier debugging/logging/and less repetitive boilerplate in the bindings and elsewhere mapping to/from user provided string values.

For example:

int get_hex_value() {
    std::string input;
    std::cin >> input;
    switch (*Color(repr)) {
      case *Color("red"):
        return 0xff0000;
      case *Color("green"):
        return 0x00ff00;
      case *Color("blue"):
        return 0x0000ff;
      default:
        std::cout << "Don't know that one; input hex value\n";
        std::cin >> std::hex >> input;
        return std::stoi(input);
    }
}

@bkietz bkietz requested a review from lidavidm July 9, 2021 19:38
@github-actions

github-actions Bot commented Jul 9, 2021

Copy link
Copy Markdown

@lidavidm lidavidm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I left some comments, but functionally, this looks good, and we can file a followup to simplify FunctionOptions with this.

Comment thread cpp/src/arrow/util/enum.h
Comment thread cpp/src/arrow/util/enum.h Outdated
Comment thread cpp/src/arrow/util/enum.h
Comment thread cpp/src/arrow/util/enum.h Outdated
Comment thread cpp/src/arrow/util/enum.h
@bkietz

bkietz commented Jul 11, 2021

Copy link
Copy Markdown
Member Author

I recommend avoiding macros whenever possible, but for completeness I'll note that we could replace

// color.h
struct Color : EnumType<Color> {
  using EnumType::EnumType;
  static constexpr const char* kValues = R"(red green blue)";
};
// color.cc
constexpr const char* Color::kValues;

with

ARROW_ENUM(Color, red green blue);

@bkietz bkietz marked this pull request as ready for review July 12, 2021 01:22
Comment thread cpp/src/arrow/util/reflection_test.cc
@lidavidm

Copy link
Copy Markdown
Member

Quick question, why did 'try replacing an enum' ultimately stay reverted? Did you just mean to leave it as a followup?

@bkietz

bkietz commented Jul 12, 2021

Copy link
Copy Markdown
Member Author

I can certainly replace one in this PR but I wasn't sure that'd be preferred. I was mostly using it as a test to ensure that the C++ builds could all pass with the replacement- if we want to include a replacement here then I'll need to repair the bindings as well

@lidavidm

Copy link
Copy Markdown
Member

Ah nah just wanted to make sure of the intent. Let's merge this? I'll file a JIRA to adjust the bindings and definitions.

@lidavidm

Copy link
Copy Markdown
Member

ARROW-13307

@bkietz

bkietz commented Jul 12, 2021

Copy link
Copy Markdown
Member Author

I want to provide one more tweak to make it easier to enable_if and provide a doccomment

@bkietz

bkietz commented Jul 12, 2021

Copy link
Copy Markdown
Member Author

but I can just append that to the follow up

@lidavidm

Copy link
Copy Markdown
Member

If you wanna do that here that's no problem

@lidavidm lidavidm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for working this out.

@lidavidm lidavidm closed this in 975f459 Jul 12, 2021
@bkietz bkietz deleted the 13296-Provide-reflection-compat branch July 12, 2021 19:05
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.

2 participants