You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "fgerlits (via GitHub)" <gi...@apache.org> on 2023/07/24 16:40:52 UTC
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1609: MINIFICPP-1817 Use magic_enum library instead of SMART_ENUM
fgerlits commented on code in PR #1609:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1609#discussion_r1272198703
##########
extensions/gcp/controllerservices/GCPCredentialsControllerService.h:
##########
@@ -30,22 +30,44 @@
#include "google/cloud/storage/oauth2/credentials.h"
namespace org::apache::nifi::minifi::extensions::gcp {
+enum class CredentialsLocation {
+ USE_DEFAULT_CREDENTIALS,
+ USE_COMPUTE_ENGINE_CREDENTIALS,
+ USE_JSON_FILE,
+ USE_JSON_CONTENTS,
+ USE_ANONYMOUS_CREDENTIALS
+};
+} // namespace org::apache::nifi::minifi::extensions::gcp
-SMART_ENUM(CredentialsLocation,
- (USE_DEFAULT_CREDENTIALS, "Google Application Default Credentials"),
- (USE_COMPUTE_ENGINE_CREDENTIALS, "Use Compute Engine Credentials"),
- (USE_JSON_FILE, "Service Account JSON File"),
- (USE_JSON_CONTENTS, "Service Account JSON"),
- (USE_ANONYMOUS_CREDENTIALS, "Use Anonymous credentials"));
+namespace magic_enum::customize {
+template <>
+constexpr customize_t enum_name<org::apache::nifi::minifi::extensions::gcp::CredentialsLocation>(org::apache::nifi::minifi::extensions::gcp::CredentialsLocation value) noexcept {
+ switch (value) {
+ case org::apache::nifi::minifi::extensions::gcp::CredentialsLocation::USE_DEFAULT_CREDENTIALS:
+ return "Google Application Default Credentials";
+ case org::apache::nifi::minifi::extensions::gcp::CredentialsLocation::USE_COMPUTE_ENGINE_CREDENTIALS:
+ return "Use Compute Engine Credentials";
+ case org::apache::nifi::minifi::extensions::gcp::CredentialsLocation::USE_JSON_FILE:
+ return "Service Account JSON File";
+ case org::apache::nifi::minifi::extensions::gcp::CredentialsLocation::USE_JSON_CONTENTS:
+ return "Service Account JSON";
+ case org::apache::nifi::minifi::extensions::gcp::CredentialsLocation::USE_ANONYMOUS_CREDENTIALS:
+ return "Use Anonymous credentials";
+ }
+ return default_tag;
Review Comment:
Should we return an `invalid_tag` in these cases? I'm not sure what, if any, the difference is, but this looks like an invalid case.
##########
extensions/azure/storage/BlobStorageClient.h:
##########
@@ -33,11 +33,29 @@
namespace org::apache::nifi::minifi::azure::storage {
-SMART_ENUM(OptionalDeletion,
- (NONE, "None"),
- (INCLUDE_SNAPSHOTS, "Include Snapshots"),
- (DELETE_SNAPSHOTS_ONLY, "Delete Snapshots Only")
-)
+enum class OptionalDeletion {
+ NONE,
+ INCLUDE_SNAPSHOTS,
+ DELETE_SNAPSHOTS_ONLY
+};
+} // namespace org::apache::nifi::minifi::azure::storage
+
+namespace magic_enum::customize {
+template <>
+constexpr customize_t enum_name<org::apache::nifi::minifi::azure::storage::OptionalDeletion>(org::apache::nifi::minifi::azure::storage::OptionalDeletion value) noexcept {
+ switch (value) {
+ case org::apache::nifi::minifi::azure::storage::OptionalDeletion::NONE:
+ return "None";
+ case org::apache::nifi::minifi::azure::storage::OptionalDeletion::INCLUDE_SNAPSHOTS:
+ return "Include Snapshots";
+ case org::apache::nifi::minifi::azure::storage::OptionalDeletion::DELETE_SNAPSHOTS_ONLY:
+ return "Delete Snapshots Only";
+ }
+ return default_tag;
+}
+} // namespace magic_enum::customize
Review Comment:
I would add either a
```c++
using OptionalDeletion = org::apache::nifi::minifi::azure::storage::OptionalDeletion;
// or a
namespace storage = org::apache::nifi::minifi::azure::storage;
```
here, and also in other blocks like this.
##########
extensions/azure/processors/ListAzureDataLakeStorage.h:
##########
@@ -33,10 +33,10 @@ class ListAzureDataLakeStorageTestsFixture;
namespace org::apache::nifi::minifi::azure {
-SMART_ENUM(EntityTracking,
- (NONE, "none"),
- (TIMESTAMPS, "timestamps")
-)
+enum class EntityTracking {
+ none,
+ timestamps
+};
Review Comment:
Is it useful to have two of these, one in `ListAzureBlobStorage` and one here, or could we merge them?
##########
libminifi/include/utils/Enum.h:
##########
@@ -17,127 +17,30 @@
#pragma once
-#include <cstring>
#include <optional>
-#include <set>
#include <string>
#include <string_view>
+#include <stdexcept>
-#include "ArrayUtils.h"
-#include "StringUtils.h"
-#include "Macro.h"
+#include "magic_enum.hpp"
namespace org::apache::nifi::minifi::utils {
-#define INCLUDE_BASE_FIELD(x) \
- x = Base::x
-
-// [[maybe_unused]] on public members to avoid warnings when used inside an anonymous namespace
-#define SMART_ENUM_BODY(Clazz, ...) \
- constexpr Clazz(Type value = static_cast<Type>(-1)) : value_{value} {} \
- explicit Clazz(std::string_view str) : value_{parse(str).value_} {} \
- explicit Clazz(const std::string& str) : Clazz(std::string_view{str}) {} \
- explicit Clazz(std::nullptr_t) = delete; \
- private: \
- Type value_; \
- public: \
- [[maybe_unused]] constexpr Type value() const { \
- return value_; \
- } \
- struct detail : public Base::detail {}; \
- static constexpr int length = Base::length + COUNT(__VA_ARGS__); \
- static constexpr auto values = org::apache::nifi::minifi::utils::array_cat(Base::values, \
- std::array<std::string_view, COUNT(__VA_ARGS__)>{FOR_EACH(SECOND, COMMA, (__VA_ARGS__))}); \
- [[maybe_unused]] const char* toString() const { \
- int index = static_cast<int>(value_); \
- return values.at(index).data(); \
- } \
- [[maybe_unused]] friend const char* toString(Type a) { \
- return Clazz{a}.toString(); \
- } \
- [[maybe_unused]] const char* toStringOr(const char* fallback) const { \
- if (*this) { \
- return toString(); \
- } \
- return fallback; \
- } \
- [[maybe_unused]] friend constexpr bool operator==(Clazz lhs, Clazz rhs) { \
- return lhs.value_ == rhs.value_; \
- } \
- [[maybe_unused]] friend constexpr bool operator!=(Clazz lhs, Clazz rhs) { \
- return lhs.value_ != rhs.value_; \
- } \
- [[maybe_unused]] friend constexpr bool operator<(Clazz lhs, Clazz rhs) { \
- return lhs.value_ < rhs.value_;\
- } \
- [[maybe_unused]] explicit constexpr operator bool() const { \
- int idx = static_cast<int>(value_); \
- return 0 <= idx && idx < length; \
- } \
- [[maybe_unused]] static Clazz parse(std::string_view str, const ::std::optional<Clazz>& fallback = {}, bool caseSensitive = true) { \
- for (int idx = 0; idx < length; ++idx) { \
- if (::org::apache::nifi::minifi::utils::StringUtils::equals(str, values.at(idx), caseSensitive)) \
- return static_cast<Type>(idx); \
- } \
- if (fallback) { \
- return fallback.value(); \
- } \
- throw std::runtime_error("Cannot convert \"" + std::string(str) + "\" to " #Clazz); \
- } \
- template<typename T, typename = typename std::enable_if<std::is_base_of<typename T::detail, detail>::value>::type> \
- [[maybe_unused]] constexpr T cast() const { \
- if (0 <= value_ && value_ < T::length) { \
- return static_cast<typename T::Type>(value_); \
- } \
- return {}; \
- }
-
-#define SMART_ENUM_OUT_OF_CLASS_DEFINITIONS(Clazz) \
- [[maybe_unused]] inline constexpr std::string_view toStringView(Clazz::Type a) { \
- int index = static_cast<int>(a); \
- if (0 <= index && index < Clazz::length) { \
- return Clazz::values.at(index); \
- } \
- return "Unknown value for " #Clazz; \
+template<typename T>
+T enumCast(std::string_view str, std::optional<T> fallback = std::nullopt, bool case_insensitive = false) {
Review Comment:
This function does two different things:
1. parse a string to an enum with fallback;
2. parse a string to an enum without fallback and throw if it fails.
I think (1) would be better done directly, as `magic_enum::enum_cast<T>(str).value_or(fallback)` (or `magic_enum::enum_cast<T>(str, magic_enum::case_insensitive).value_or(fallback)`). Then this function would only do (2), and the `fallback` parameter could be removed.
##########
libminifi/include/utils/ProcessorConfigUtils.h:
##########
@@ -39,32 +40,27 @@ std::vector<std::string> listFromCommaSeparatedProperty(const core::ProcessConte
std::vector<std::string> listFromRequiredCommaSeparatedProperty(const core::ProcessContext& context, std::string_view property_name);
bool parseBooleanPropertyOrThrow(const core::ProcessContext& context, std::string_view property_name);
std::chrono::milliseconds parseTimePropertyMSOrThrow(const core::ProcessContext& context, std::string_view property_name);
-std::string parsePropertyWithAllowableValuesOrThrow(const core::ProcessContext& context, std::string_view property_name, std::span<const std::string_view> allowable_values);
template<typename T>
-T parseEnumProperty(const core::ProcessContext& context, const core::Property& prop) {
+T parseEnumProperty(const core::ProcessContext& context, const core::PropertyReference& prop) {
std::string value;
- if (!context.getProperty(prop.getName(), value)) {
- throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Property '" + prop.getName() + "' is missing");
+ if (!context.getProperty(prop.name, value)) {
+ throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Property '" + std::string(prop.name) + "' is missing");
}
- T result = T::parse(value.c_str(), T{});
+ auto result = magic_enum::enum_cast<T>(value);
if (!result) {
- throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Property '" + prop.getName() + "' has invalid value: '" + value + "'");
+ throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Property '" + std::string(prop.name) + "' has invalid value: '" + value + "'");
}
- return result;
+ return result.value();
}
template<typename T>
-T parseEnumProperty(const core::ProcessContext& context, const core::PropertyReference& prop) {
+std::optional<T> parseOptionalEnumProperty(const core::ProcessContext& context, const core::PropertyReference& prop) {
std::string value;
if (!context.getProperty(prop.name, value)) {
- throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Property '" + std::string(prop.name) + "' is missing");
- }
- T result = T::parse(value.c_str(), T{});
- if (!result) {
- throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Property '" + std::string(prop.name) + "' has invalid value: '" + value + "'");
+ return std::nullopt;
}
- return result;
+ return magic_enum::enum_cast<T>(value);
Review Comment:
I think we used to throw if a value is set but is not parseable as a T, and we should keep that behavior.
##########
libminifi/include/utils/Enum.h:
##########
@@ -17,127 +17,30 @@
#pragma once
-#include <cstring>
#include <optional>
-#include <set>
#include <string>
#include <string_view>
+#include <stdexcept>
-#include "ArrayUtils.h"
-#include "StringUtils.h"
-#include "Macro.h"
+#include "magic_enum.hpp"
namespace org::apache::nifi::minifi::utils {
-#define INCLUDE_BASE_FIELD(x) \
- x = Base::x
-
-// [[maybe_unused]] on public members to avoid warnings when used inside an anonymous namespace
-#define SMART_ENUM_BODY(Clazz, ...) \
- constexpr Clazz(Type value = static_cast<Type>(-1)) : value_{value} {} \
- explicit Clazz(std::string_view str) : value_{parse(str).value_} {} \
- explicit Clazz(const std::string& str) : Clazz(std::string_view{str}) {} \
- explicit Clazz(std::nullptr_t) = delete; \
- private: \
- Type value_; \
- public: \
- [[maybe_unused]] constexpr Type value() const { \
- return value_; \
- } \
- struct detail : public Base::detail {}; \
- static constexpr int length = Base::length + COUNT(__VA_ARGS__); \
- static constexpr auto values = org::apache::nifi::minifi::utils::array_cat(Base::values, \
- std::array<std::string_view, COUNT(__VA_ARGS__)>{FOR_EACH(SECOND, COMMA, (__VA_ARGS__))}); \
- [[maybe_unused]] const char* toString() const { \
- int index = static_cast<int>(value_); \
- return values.at(index).data(); \
- } \
- [[maybe_unused]] friend const char* toString(Type a) { \
- return Clazz{a}.toString(); \
- } \
- [[maybe_unused]] const char* toStringOr(const char* fallback) const { \
- if (*this) { \
- return toString(); \
- } \
- return fallback; \
- } \
- [[maybe_unused]] friend constexpr bool operator==(Clazz lhs, Clazz rhs) { \
- return lhs.value_ == rhs.value_; \
- } \
- [[maybe_unused]] friend constexpr bool operator!=(Clazz lhs, Clazz rhs) { \
- return lhs.value_ != rhs.value_; \
- } \
- [[maybe_unused]] friend constexpr bool operator<(Clazz lhs, Clazz rhs) { \
- return lhs.value_ < rhs.value_;\
- } \
- [[maybe_unused]] explicit constexpr operator bool() const { \
- int idx = static_cast<int>(value_); \
- return 0 <= idx && idx < length; \
- } \
- [[maybe_unused]] static Clazz parse(std::string_view str, const ::std::optional<Clazz>& fallback = {}, bool caseSensitive = true) { \
- for (int idx = 0; idx < length; ++idx) { \
- if (::org::apache::nifi::minifi::utils::StringUtils::equals(str, values.at(idx), caseSensitive)) \
- return static_cast<Type>(idx); \
- } \
- if (fallback) { \
- return fallback.value(); \
- } \
- throw std::runtime_error("Cannot convert \"" + std::string(str) + "\" to " #Clazz); \
- } \
- template<typename T, typename = typename std::enable_if<std::is_base_of<typename T::detail, detail>::value>::type> \
- [[maybe_unused]] constexpr T cast() const { \
- if (0 <= value_ && value_ < T::length) { \
- return static_cast<typename T::Type>(value_); \
- } \
- return {}; \
- }
-
-#define SMART_ENUM_OUT_OF_CLASS_DEFINITIONS(Clazz) \
- [[maybe_unused]] inline constexpr std::string_view toStringView(Clazz::Type a) { \
- int index = static_cast<int>(a); \
- if (0 <= index && index < Clazz::length) { \
- return Clazz::values.at(index); \
- } \
- return "Unknown value for " #Clazz; \
+template<typename T>
+T enumCast(std::string_view str, std::optional<T> fallback = std::nullopt, bool case_insensitive = false) {
+ std::optional<T> enum_optional_value;
+ if (case_insensitive) {
+ enum_optional_value = magic_enum::enum_cast<T>(str, magic_enum::case_insensitive);
+ } else {
+ enum_optional_value = magic_enum::enum_cast<T>(str);
}
-
-/**
- * These macros provide an encapsulation of enum-like behavior offering the following:
- * - switch safety: the compiler can detect if some enum cases are not handled
- * - string conversion: convert between enum instances and their string representations (toString, parse)
- * - validity: check if it contains a value that is an invalid enum value
- * - extensibility: extend an enum with new values and safely* cast from derived to base
- * (* "safely" here means that there is no casting between unrelated enums)
- * - reflection: access the set of all string representations
- */
-
-#define SMART_ENUM(Clazz, ...) \
- struct Clazz { \
- using Base = ::org::apache::nifi::minifi::utils::EnumBase; \
- enum Type : int { \
- FOR_EACH(FIRST, COMMA, (__VA_ARGS__)) \
- }; \
- SMART_ENUM_BODY(Clazz, __VA_ARGS__) \
- }; \
- SMART_ENUM_OUT_OF_CLASS_DEFINITIONS(Clazz)
-
-#define SMART_ENUM_EXTEND(Clazz, base, base_fields, ...) \
- struct Clazz { \
- using Base = base; \
- enum Type : int { \
- FOR_EACH(INCLUDE_BASE_FIELD, COMMA, base_fields), \
- FOR_EACH(FIRST, COMMA, (__VA_ARGS__)) \
- }; \
- static_assert((COUNT base_fields) == Base::length, "Must enumerate all base instance values"); \
- SMART_ENUM_BODY(Clazz, __VA_ARGS__) \
- }; \
- SMART_ENUM_OUT_OF_CLASS_DEFINITIONS(Clazz)
-
-struct EnumBase {
- enum Type {};
- static constexpr int length = 0;
- static constexpr std::array<std::string_view, 0> values = {};
- struct detail {};
-};
+ if (enum_optional_value) {
+ return enum_optional_value.value();
+ }
+ if (fallback) {
+ return *fallback;
+ }
+ throw std::runtime_error("Cannot convert \"" + std::string(str) + "\" to enum class value");
Review Comment:
It would be good to print the type of the enum in this message, too. We could use either `core::className()` or `magic_enum::enum_type_name()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org