[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246969665
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
+ using org::apache::nifi::minifi::core::Property;
+
+ uint64_t uint64_var = 0;
+ REQUIRE(Property::StringToInt("-2", uint64_var) == false); // Negative
shouldn't be converted to uint
+
+ uint32_t uint32_var = 0;
+ REQUIRE(Property::StringToInt("3 GB", uint32_var) == true); // Test
skipping of spaces, too
+ uint32_t expected_value = 3u * 1024 * 1024 * 1024;
+ REQUIRE(uint32_var == expected_value);
+
+ int32_t int32_var = 0;
+ REQUIRE(Property::StringToInt("3GB", int32_var) == false); // Doesn't fit
+
+ REQUIRE(Property::StringToInt("-1 G", int32_var) == true);
+ REQUIRE(int32_var == -1 * 1000 * 1000 * 1000);
+
+ REQUIRE(Property::StringToInt("-1G", uint32_var) == false); // Negative to
uint
Review comment:
This shouldn't be allowed. Whether this barrier exists here or elsewhere is
what I think we should discuss. It's about the end to end correctness, so not
sure if the appropriate action is to fix the signedness issues or correct the
overall issue that 707 was hoping to look into. I fear that resolving any
issues like signed values without looking into solving the whole picture takes
us to an unknown state. Thoughts?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246974404
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
+ using org::apache::nifi::minifi::core::Property;
+
+ uint64_t uint64_var = 0;
+ REQUIRE(Property::StringToInt("-2", uint64_var) == false); // Negative
shouldn't be converted to uint
Review comment:
I realize 710 kind of had a different focus, but your comment on the prior
PR in which you mentioned this was to open 707 in hopes of opening a discussion
for better dealing with this. For example, it may be the case where sometimes a
negative value makes sense AND the conversion make sense. I will do some
research, as I know these cases exist. In others, that is NOT true.
While the above test is totally valid from the perspective of 710, the hope
was that we could discuss and more intelligently handle these cases, perhaps
with the validators themselves. What is your perspective?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246858533
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
Review comment:
Almost all of these are invalid and shouldn't be accepted, though (not the
changes -- the actual entries like - 2 GB ). This was the point of
https://issues.apache.org/jira/browse/MINIFICPP-707. I don't want to trounce on
this PR, but the reason I created the ticket was that some of these SHOULD be
prevented from being used at all in configs, fail early. Therefore it's not as
simple as doing what is done here. There needs to be more thought put into
this, this was the primary reason I created the ticket versus implementing.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246858533
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
Review comment:
Almost all of these are invalid and shouldn't be accepted, though (not the
changes -- the actual entries ). This was the point of
https://issues.apache.org/jira/browse/MINIFICPP-707. I don't want to trounce on
this PR, but the reason I created the ticket was that some of these SHOULD be
prevented from being used at all in configs, fail early. Therefore it's not as
simple as doing what is done here. 710 should be OBE in favor of 707. There
needs to be more thought put into this, this was the primary reason I created
the ticket versus implementing.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246976098
##
File path: libminifi/include/core/TypedValues.h
##
@@ -141,6 +141,47 @@ class TimePeriodValue : public TransformableValue, public
state::response::UInt6
* format .
*/
class DataSizeValue : public TransformableValue, public
state::response::UInt64Value {
+private:
+
+ //Signed
+ template::value,
T>::type* = nullptr>
+ static bool AssignIfFits(int64_t base, uint64_t mult, T& output) {
+int64_t val = base * mult;
+if(val >= std::numeric_limits::min() && val <=
std::numeric_limits::max()) {
+ output = val;
+ return true;
+}
+return false;
+ }
+
+ //Unsigned
+ template::value,
T>::type* = nullptr>
+ static bool AssignIfFits(uint64_t base, uint64_t mult, T& output) {
+uint64_t val = base * mult;
+if(val <= std::numeric_limits::max()) {
+ output = val;
+ return true;
+}
+return false;
+ }
+
+ //Signed
+ template::value,
T>::type* = nullptr>
+ static bool StringToNum(const char* str_begin, char **str_end, T& output) {
+int64_t val = std::strtoll(str_begin, str_end, 0);
+return AssignIfFits(val, 1, output);
Review comment:
why have any multiplication at all if it is 1? same with the one, below.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246976098
##
File path: libminifi/include/core/TypedValues.h
##
@@ -141,6 +141,47 @@ class TimePeriodValue : public TransformableValue, public
state::response::UInt6
* format .
*/
class DataSizeValue : public TransformableValue, public
state::response::UInt64Value {
+private:
+
+ //Signed
+ template::value,
T>::type* = nullptr>
+ static bool AssignIfFits(int64_t base, uint64_t mult, T& output) {
+int64_t val = base * mult;
+if(val >= std::numeric_limits::min() && val <=
std::numeric_limits::max()) {
+ output = val;
+ return true;
+}
+return false;
+ }
+
+ //Unsigned
+ template::value,
T>::type* = nullptr>
+ static bool AssignIfFits(uint64_t base, uint64_t mult, T& output) {
+uint64_t val = base * mult;
+if(val <= std::numeric_limits::max()) {
+ output = val;
+ return true;
+}
+return false;
+ }
+
+ //Signed
+ template::value,
T>::type* = nullptr>
+ static bool StringToNum(const char* str_begin, char **str_end, T& output) {
+int64_t val = std::strtoll(str_begin, str_end, 0);
+return AssignIfFits(val, 1, output);
Review comment:
why have any multiplication at all if it is 1?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246971930
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
Review comment:
I didn't think my last comment was entirely helpful so I went through and
tried to parse out the parts that are appropriate and SHOULD be accepted. In
retrospect I think I was being a bit terse jumping between tasks. Sorry if that
comment didn't provide much insight into my thought process.
Hopefully I was more clear on what we can immediately accept and what I
think needs more discussion. I'm totally okay if that discussion yields pushing
some of the concerns down the road if it makes sense.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246969665
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
+ using org::apache::nifi::minifi::core::Property;
+
+ uint64_t uint64_var = 0;
+ REQUIRE(Property::StringToInt("-2", uint64_var) == false); // Negative
shouldn't be converted to uint
+
+ uint32_t uint32_var = 0;
+ REQUIRE(Property::StringToInt("3 GB", uint32_var) == true); // Test
skipping of spaces, too
+ uint32_t expected_value = 3u * 1024 * 1024 * 1024;
+ REQUIRE(uint32_var == expected_value);
+
+ int32_t int32_var = 0;
+ REQUIRE(Property::StringToInt("3GB", int32_var) == false); // Doesn't fit
+
+ REQUIRE(Property::StringToInt("-1 G", int32_var) == true);
+ REQUIRE(int32_var == -1 * 1000 * 1000 * 1000);
+
+ REQUIRE(Property::StringToInt("-1G", uint32_var) == false); // Negative to
uint
Review comment:
This shouldn't be allowed. Whether this barrier exists here or elsewhere is
what I think we should discuss. It's about the end to end correctness, so not
sure if the appropriate action is to fix the signedness issues or correct the
overall issue that 707 was hoping to look into. Thoughts?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246974404
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
+ using org::apache::nifi::minifi::core::Property;
+
+ uint64_t uint64_var = 0;
+ REQUIRE(Property::StringToInt("-2", uint64_var) == false); // Negative
shouldn't be converted to uint
Review comment:
I realize 710 kind of had a different focus, but your comment on the prior
PR in which you mentioned this was to open 707 in hopes of opening a discussion
for better dealing with this. For example, it may be the case where sometimes a
negative value makes sense AND the conversion make sense. I will do some
research, as I know these cases exist. In others, that is NOT true.
While the above test is totally valid from the perspective of 710, the hope
was that we could discuss and more intelligently handle these cases, perhaps
with the validators themselves. What is your perspective? Would you rather fix
the low hanging fruit and move on or discuss alternatives?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246973810
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
+ using org::apache::nifi::minifi::core::Property;
+
+ uint64_t uint64_var = 0;
+ REQUIRE(Property::StringToInt("-2", uint64_var) == false); // Negative
shouldn't be converted to uint
+
+ uint32_t uint32_var = 0;
+ REQUIRE(Property::StringToInt("3 GB", uint32_var) == true); // Test
skipping of spaces, too
Review comment:
Some of these tests exist in yaml if I recall correctly ( but I didn't write
them ), if you're interested.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246972159
##
File path: libminifi/include/core/TypedValues.h
##
@@ -141,6 +141,47 @@ class TimePeriodValue : public TransformableValue, public
state::response::UInt6
* format .
*/
class DataSizeValue : public TransformableValue, public
state::response::UInt64Value {
+private:
+
+ //Signed
+ template::value,
T>::type* = nullptr>
+ static bool AssignIfFits(int64_t base, uint64_t mult, T& output) {
+int64_t val = base * mult;
+if(val >= std::numeric_limits::min() && val <=
std::numeric_limits::max()) {
+ output = val;
+ return true;
+}
+return false;
+ }
+
Review comment:
Appveyor showed that it didn't. Do you have access to windows? I can
probably help with that since I do.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246971930
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
Review comment:
I didn't think my last comment was entirely helpful so I went through and
tried to parse out the parts that are appropriate and SHOULD be accepted. In
retrospect I think I was being a bit terse jumping between tasks. Sorry if that
comment didn't provide much insight into my thought process.
Hopefully I was more clear on what we can immediately and what we can't
accept without more discussion. If we can parse this out then we can approve
and merge the good parts.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246971930
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
Review comment:
I didn't think my last comment was entirely helpful so I went through and
tried to parse out the parts that are appropriate and SHOULD be accepted.
Hopefully I was more clear on what we can immediately and what we can't accept
without more discussion. If we can parse this out then we can approve and merge
the good parts.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246971618
##
File path: libminifi/include/core/TypedValues.h
##
@@ -141,6 +141,47 @@ class TimePeriodValue : public TransformableValue, public
state::response::UInt6
* format .
*/
class DataSizeValue : public TransformableValue, public
state::response::UInt64Value {
+private:
+
+ //Signed
+ template::value,
T>::type* = nullptr>
+ static bool AssignIfFits(int64_t base, uint64_t mult, T& output) {
+int64_t val = base * mult;
+if(val >= std::numeric_limits::min() && val <=
std::numeric_limits::max()) {
+ output = val;
+ return true;
+}
+return false;
+ }
+
Review comment:
Does this code compile in windows? I'm not sure that it will.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246971581
##
File path: libminifi/include/core/TypedValues.h
##
@@ -141,6 +141,47 @@ class TimePeriodValue : public TransformableValue, public
state::response::UInt6
* format .
*/
class DataSizeValue : public TransformableValue, public
state::response::UInt64Value {
+private:
+
+ //Signed
+ template::value,
T>::type* = nullptr>
+ static bool AssignIfFits(int64_t base, uint64_t mult, T& output) {
+int64_t val = base * mult;
+if(val >= std::numeric_limits::min() && val <=
std::numeric_limits::max()) {
+ output = val;
+ return true;
+}
+return false;
+ }
+
+ //Unsigned
+ template::value,
T>::type* = nullptr>
+ static bool AssignIfFits(uint64_t base, uint64_t mult, T& output) {
+uint64_t val = base * mult;
+if(val <= std::numeric_limits::max()) {
+ output = val;
+ return true;
+}
+return false;
+ }
+
+ //Signed
+ template::value,
T>::type* = nullptr>
+ static bool StringToNum(const char* str_begin, char **str_end, T& output) {
+int64_t val = std::strtoll(str_begin, str_end, 0);
+return AssignIfFits(val, 1, output);
+ }
+
+ //Unsigned
+ template::value,
T>::type* = nullptr>
+ static bool StringToNum(const char* str_begin, char **str_end, T& output) {
+if(str_begin[0] == '-') {
Review comment:
can't assume that it's zero, and that it's non-empty
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246971559
##
File path: libminifi/src/c2/protocols/RESTProtocol.cpp
##
@@ -129,7 +129,7 @@ const C2Payload RESTProtocol::parseJsonResponse(const
C2Payload , const
} catch (...) {
}
#endif
- return std::move(C2Payload(payload.getOperation(),
state::UpdateState::READ_COMPLETE, true));
Review comment:
Good catch. I rarely build with clang these days...
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246971275
##
File path: libminifi/include/core/TypedValues.h
##
@@ -163,7 +204,11 @@ class DataSizeValue : public TransformableValue, public
state::response::UInt64V
const char *cvalue = input.c_str();
char *pEnd;
-auto ival = std::strtoll(cvalue, , 0);
+
+T ival = 0;
+if(!StringToNum(cvalue, , ival)) {
Review comment:
This is only applicable if we reach the null terminator and it is not a byte
string
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246970833
##
File path: libminifi/include/core/TypedValues.h
##
@@ -141,6 +141,47 @@ class TimePeriodValue : public TransformableValue, public
state::response::UInt6
* format .
*/
class DataSizeValue : public TransformableValue, public
state::response::UInt64Value {
+private:
+
+ //Signed
+ template::value,
T>::type* = nullptr>
+ static bool AssignIfFits(int64_t base, uint64_t mult, T& output) {
Review comment:
These I like and think we should use this.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246969665
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
+ using org::apache::nifi::minifi::core::Property;
+
+ uint64_t uint64_var = 0;
+ REQUIRE(Property::StringToInt("-2", uint64_var) == false); // Negative
shouldn't be converted to uint
+
+ uint32_t uint32_var = 0;
+ REQUIRE(Property::StringToInt("3 GB", uint32_var) == true); // Test
skipping of spaces, too
+ uint32_t expected_value = 3u * 1024 * 1024 * 1024;
+ REQUIRE(uint32_var == expected_value);
+
+ int32_t int32_var = 0;
+ REQUIRE(Property::StringToInt("3GB", int32_var) == false); // Doesn't fit
+
+ REQUIRE(Property::StringToInt("-1 G", int32_var) == true);
+ REQUIRE(int32_var == -1 * 1000 * 1000 * 1000);
+
+ REQUIRE(Property::StringToInt("-1G", uint32_var) == false); // Negative to
uint
Review comment:
This shouldn't be allowed. Whether this barrier exists here or elsewhere is
what I think we should discuss. It's about the end to end correctness.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246969601
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
+ using org::apache::nifi::minifi::core::Property;
+
+ uint64_t uint64_var = 0;
+ REQUIRE(Property::StringToInt("-2", uint64_var) == false); // Negative
shouldn't be converted to uint
+
+ uint32_t uint32_var = 0;
+ REQUIRE(Property::StringToInt("3 GB", uint32_var) == true); // Test
skipping of spaces, too
+ uint32_t expected_value = 3u * 1024 * 1024 * 1024;
+ REQUIRE(uint32_var == expected_value);
+
+ int32_t int32_var = 0;
+ REQUIRE(Property::StringToInt("3GB", int32_var) == false); // Doesn't fit
Review comment:
One of the things that the big PR left as a todo was to write tests with
conversions, so I will readily admit that there may be some cleanup.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246969481
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
+ using org::apache::nifi::minifi::core::Property;
+
+ uint64_t uint64_var = 0;
+ REQUIRE(Property::StringToInt("-2", uint64_var) == false); // Negative
shouldn't be converted to uint
+
+ uint32_t uint32_var = 0;
+ REQUIRE(Property::StringToInt("3 GB", uint32_var) == true); // Test
skipping of spaces, too
+ uint32_t expected_value = 3u * 1024 * 1024 * 1024;
+ REQUIRE(uint32_var == expected_value);
+
+ int32_t int32_var = 0;
+ REQUIRE(Property::StringToInt("3GB", int32_var) == false); // Doesn't fit
+
+ REQUIRE(Property::StringToInt("-1 G", int32_var) == true);
Review comment:
We should almost exclusively not allow signed Byte values in string to int.
It should always be positive.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246969407
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
+ using org::apache::nifi::minifi::core::Property;
+
+ uint64_t uint64_var = 0;
+ REQUIRE(Property::StringToInt("-2", uint64_var) == false); // Negative
shouldn't be converted to uint
+
+ uint32_t uint32_var = 0;
+ REQUIRE(Property::StringToInt("3 GB", uint32_var) == true); // Test
skipping of spaces, too
+ uint32_t expected_value = 3u * 1024 * 1024 * 1024;
+ REQUIRE(uint32_var == expected_value);
+
+ int32_t int32_var = 0;
+ REQUIRE(Property::StringToInt("3GB", int32_var) == false); // Doesn't fit
Review comment:
I'll have to look at the changes more closely but we shouldn't be worrying
about int32, we're almost exclusively uint64 and when we're not ( such as
someone saying a type is int ), we should be okay up-converting. It's possible
that someone sets a type, but I don't think we can say at this prospect if the
type fits or doesn't fit
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246858533
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
Review comment:
Almost all of these are invalid and shouldn't be accepted, though. This was
the point of https://issues.apache.org/jira/browse/MINIFICPP-707. I don't want
to trounce on this PR, but the reason I created the ticket was that some of
these SHOULD be prevented from being used at all in configs, fail early.
Therefore it's not as simple as doing what is done here. 710 should be OBE in
favor of 707. There needs to be more thought put into this, this was the
primary reason I created the ticket versus implementing.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246858533
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
Review comment:
Almost all of these are invalid and shouldn't be accepted, though. This was
the point of https://issues.apache.org/jira/browse/MINIFICPP-707. I don't want
to trounce on this PR, but the reason I created the ticket was that some of
these SHOULD be prevented from being used at all in configs, fail early.
Therefore it's not as simple as doing what is done here. 710 should be OBE in
favor of 707. There needs to be more thought put into this.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors in Property StringToInt conversion
phrocker commented on a change in pull request #472: MINIFICPP-710 - Fix errors
in Property StringToInt conversion
URL: https://github.com/apache/nifi-minifi-cpp/pull/472#discussion_r246858533
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
REQUIRE(test.c_str()[1] == ' ');
}
+TEST_CASE("Test int conversion", "[testStringToInt]") {
Review comment:
Almost all of these are invalid and shouldn't be accepted, though. This was
the point of https://issues.apache.org/jira/browse/MINIFICPP-707. I don't want
to trounce on this PR, but the reason I created the ticket was that some of
these SHOULD be prevented from being used at all in configs, fail early.
Therefore it's not as simple as doing what is done here. 710 should be OBE in
favor of 707.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
