szaszm commented on code in PR #1626:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1626#discussion_r1290183175
##########
libminifi/include/core/PropertyDefinitionBuilder.h:
##########
@@ -23,75 +23,69 @@
namespace org::apache::nifi::minifi::core {
-template <size_t NumAllowedValues = 0, size_t NumAllowedTypes = 0, size_t
NumDependentProperties = 0, size_t NumExclusiveOfProperties = 0>
+template<size_t NumAllowedValues = 0, size_t NumDependentProperties = 0,
size_t NumExclusiveOfProperties = 0, typename AllowedTypes =
utils::meta::type_list<>>
struct PropertyDefinitionBuilder {
- static constexpr PropertyDefinitionBuilder<NumAllowedValues,
NumAllowedTypes, NumDependentProperties, NumExclusiveOfProperties>
createProperty(std::string_view name) {
- PropertyDefinitionBuilder<NumAllowedValues, NumAllowedTypes,
NumDependentProperties, NumExclusiveOfProperties> builder;
+ static constexpr PropertyDefinitionBuilder<NumAllowedValues,
NumDependentProperties, NumExclusiveOfProperties, AllowedTypes>
createProperty(std::string_view name) {
+ PropertyDefinitionBuilder<NumAllowedValues, NumDependentProperties,
NumExclusiveOfProperties, AllowedTypes> builder;
builder.property.name = name;
return builder;
}
- static constexpr PropertyDefinitionBuilder<NumAllowedValues,
NumAllowedTypes, NumDependentProperties, NumExclusiveOfProperties>
createProperty(std::string_view name, std::string_view display_name) {
- PropertyDefinitionBuilder<NumAllowedValues, NumAllowedTypes,
NumDependentProperties, NumExclusiveOfProperties> builder;
+ static constexpr PropertyDefinitionBuilder<NumAllowedValues,
NumDependentProperties, NumExclusiveOfProperties, AllowedTypes>
createProperty(std::string_view name, std::string_view display_name) {
+ PropertyDefinitionBuilder<NumAllowedValues, NumDependentProperties,
NumExclusiveOfProperties, AllowedTypes> builder;
builder.property.name = name;
builder.property.display_name = display_name;
return builder;
}
- constexpr PropertyDefinitionBuilder<NumAllowedValues, NumAllowedTypes,
NumDependentProperties, NumExclusiveOfProperties>
withDescription(std::string_view description) {
+ constexpr PropertyDefinitionBuilder<NumAllowedValues,
NumDependentProperties, NumExclusiveOfProperties, AllowedTypes>
withDescription(std::string_view description) {
property.description = description;
return *this;
}
- constexpr PropertyDefinitionBuilder<NumAllowedValues, NumAllowedTypes,
NumDependentProperties, NumExclusiveOfProperties> isRequired(bool required) {
+ constexpr PropertyDefinitionBuilder<NumAllowedValues,
NumDependentProperties, NumExclusiveOfProperties, AllowedTypes> isRequired(bool
required) {
property.is_required = required;
return *this;
}
- constexpr PropertyDefinitionBuilder<NumAllowedValues, NumAllowedTypes,
NumDependentProperties, NumExclusiveOfProperties>
supportsExpressionLanguage(bool supports_expression_language) {
+ constexpr PropertyDefinitionBuilder<NumAllowedValues,
NumDependentProperties, NumExclusiveOfProperties, AllowedTypes>
supportsExpressionLanguage(bool supports_expression_language) {
property.supports_expression_language = supports_expression_language;
return *this;
}
- constexpr PropertyDefinitionBuilder<NumAllowedValues, NumAllowedTypes,
NumDependentProperties, NumExclusiveOfProperties>
withDefaultValue(std::string_view default_value) {
+ constexpr PropertyDefinitionBuilder<NumAllowedValues,
NumDependentProperties, NumExclusiveOfProperties, AllowedTypes>
withDefaultValue(std::string_view default_value) {
property.default_value = std::optional<std::string_view>{default_value};
// workaround for gcc 11.1; on gcc 11.3 and later, `property.default_value =
default_value` works, too
return *this;
}
- constexpr PropertyDefinitionBuilder<NumAllowedValues, NumAllowedTypes,
NumDependentProperties, NumExclusiveOfProperties> withAllowedValues(
+ constexpr PropertyDefinitionBuilder<NumAllowedValues,
NumDependentProperties, NumExclusiveOfProperties, AllowedTypes>
withAllowedValues(
std::array<std::string_view, NumAllowedValues> allowed_values) {
property.allowed_values = allowed_values;
return *this;
}
- constexpr PropertyDefinitionBuilder<NumAllowedValues, NumAllowedTypes,
NumDependentProperties, NumExclusiveOfProperties> withAllowedTypes(
- std::array<std::string_view, NumAllowedTypes> types) {
- property.allowed_types = types;
- return *this;
- }
Review Comment:
I think it would be nicer to not have to specify this upfront in the
template argument list, only in the call. I think something like this would
work:
```cpp
template<typename... NewAllowedTypes>
constexpr PropertyDefinitionBuilder<NumAllowedValues,
NumDependentProperties, NumExclusiveProperties,
utils::meta::type_list<NewAllowedTypes...>> withAllowedTypes()
{
return {
.property = {
.name = property.name,
.display_name = property.display_name,
.description = property.description,
.is_required = property.is_required,
.allowed_values = property.allowed_values;
.dependent_properties = property.dependent_properties,
.exclusive_of_properties = property.exclusive_of_properties,
.default_value = property.default_value,
.type = property.type,
.supports_expression_language = property.supports_expression_language
}
};
}
```
Optionally, the member list duplication could be moved by defining a
templated copy constructor on `PropertyDefinition`, or it can be transformed
into some additional complexity by using X macro or similar reflection tricks
to list all members of `PropertyDefinition`. The second approach should make it
easier to do the same with the rest of the template arguments as well.
```cpp
// near PropertyDefinition
#define PROPERTY_DEFINITION_MEMBER_LIST (name, display_name, description,
is_required, allowed_values, dependent_properties, exclusive_of_properties,
default_value, type, supports_expression_language)
// in PropertyDefinitionBuilder, using utils/Macro.h
template<typename... NewAllowedTypes>
constexpr PropertyDefinitionBuilder<NumAllowedValues,
NumDependentProperties, NumExclusiveProperties,
utils::meta::type_list<NewAllowedTypes...>> withAllowedTypes()
{
#define
PROPERTY_DEFINITION_BUILDER_WITH_ALLOWED_TYPES_IMPL_COPY_MEMBER(member, ...)
.member = property.member
return {
.property = {
FOR_EACH(PROPERTY_DEFINITION_BUILDER_WITH_ALLOWED_TYPES_IMPL_COPY_MEMBER,
COMMA, PROPERTY_DEFINITION_MEMBER_LIST)
}
};
#undef PROPERTY_DEFINITION_BUILDER_WITH_ALLOWED_TYPES_COPY_MEMBER
}
```
Both of these examples are untested, so they may have bugs. Especially the
second one, because I'm not familiar with all of the preprocessor quirks.
##########
libminifi/include/utils/meta/type_list.h:
##########
@@ -15,14 +15,23 @@
* limitations under the License.
*/
#pragma once
+
+#include <array>
+#include <string_view>
#include <type_traits>
+#include "core/Core.h"
+
namespace org::apache::nifi::minifi::utils::meta {
+
template<typename... Types>
struct type_list {
template<typename T>
[[nodiscard]] constexpr static bool contains() noexcept {
return (std::is_same_v<T, Types> || ...);
}
+
+ static constexpr auto AsStringViews = std::array<std::string_view,
sizeof...(Types)>{core::className<Types>()...};
Review Comment:
I would either move this near the usage site, or keep it more generic.
Possible generic version:
```suggestion
template<template<typename...> typename Container, typename Mapper>
requires (requires(Mapper m) { m<Types>(); } && ...)
static constexpr auto MapToValues(Mapper templated_map_function) {
return Container{templated_map_function<Types>()...};
}
// MapToValues<std::array>([]<typename T>() { return core::className<T>();
})
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
