You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "szaszm (via GitHub)" <gi...@apache.org> on 2023/09/16 03:17:48 UTC
[GitHub] [nifi-minifi-cpp] szaszm opened a new pull request, #1659: MINIFICPP-2218 Refactor expected monadic functions
szaszm opened a new pull request, #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659
- rename flatMap -> andThen, map -> transform, to match the standardized names
- implement transformError to match the standardized expected monadic interface
- simplified expected monadic function implementations
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [x] If applicable, have you updated the LICENSE file?
- [x] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [x] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1329684687
##########
libminifi/include/utils/expected.h:
##########
@@ -31,28 +29,42 @@ inline constexpr bool is_expected_v = false;
template<typename V, typename E>
inline constexpr bool is_expected_v<nonstd::expected<V, E>> = true;
-// map implementation
-template<typename Expected, typename F, typename = std::enable_if_t<is_expected_v<utils::remove_cvref_t<Expected>>>>
-auto operator|(Expected&& object, map_wrapper<F> f) {
- using value_type = typename utils::remove_cvref_t<Expected>::value_type;
- using error_type = typename utils::remove_cvref_t<Expected>::error_type;
- if constexpr (std::is_same_v<value_type, void>) {
- using function_return_type = std::decay_t<decltype(std::invoke(std::forward<F>(f.function)))>;
+template<typename T>
+concept expected = is_expected_v<std::remove_cvref_t<T>>;
+
+template<typename T>
+inline constexpr bool is_valid_unexpected_type_v = std::is_same_v<T, std::remove_cvref_t<T>> && std::is_object_v<T> && !std::is_array_v<T>;
+
+template<typename T>
+inline constexpr bool is_valid_unexpected_type_v<nonstd::unexpected_type<T>> = false;
Review Comment:
The way I see it, the `T`s in lines 35 and 41 refer to the same thing, and if we rewrite this template to use `E`, then `T = nonstd::unexpected_type<E>`. Using `T` here makes it unclear that this is a template specialization.
If you think using three different template variables is better, that's fine, too.
--
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1329598353
##########
libminifi/test/unit/ExpectedTest.cpp:
##########
@@ -484,3 +484,63 @@ TEST_CASE("expected valueOrElse", "[expected][valueOrElse]") {
REQUIRE_THROWS_AS(ex | utils::valueOrElse([](const std::string&) -> int { throw std::exception(); }), std::exception);
REQUIRE_THROWS_AS(std::move(ex) | utils::valueOrElse([](std::string&&) -> int { throw std::exception(); }), std::exception);
}
+
+TEST_CASE("expected transformError", "[expected][transformError]") {
+ auto mul2 = [](int a) { return a * 2; };
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2); // NOLINT(performance-move-const-arg)
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
Review Comment:
It doesn't make much sense to use const rvalue refs, but this still checks that the implementation matches the expectations in this unlikely case. I just copy pasted the transform tests from above, which were copy pasted from TartanLlama's test suite.
How would you structure the suggested new test case, that ends up testing the expected implementation, and not the C++ language itself?
--
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1329755886
##########
libminifi/include/utils/expected.h:
##########
@@ -31,28 +29,42 @@ inline constexpr bool is_expected_v = false;
template<typename V, typename E>
inline constexpr bool is_expected_v<nonstd::expected<V, E>> = true;
-// map implementation
-template<typename Expected, typename F, typename = std::enable_if_t<is_expected_v<utils::remove_cvref_t<Expected>>>>
-auto operator|(Expected&& object, map_wrapper<F> f) {
- using value_type = typename utils::remove_cvref_t<Expected>::value_type;
- using error_type = typename utils::remove_cvref_t<Expected>::error_type;
- if constexpr (std::is_same_v<value_type, void>) {
- using function_return_type = std::decay_t<decltype(std::invoke(std::forward<F>(f.function)))>;
+template<typename T>
+concept expected = is_expected_v<std::remove_cvref_t<T>>;
+
+template<typename T>
+inline constexpr bool is_valid_unexpected_type_v = std::is_same_v<T, std::remove_cvref_t<T>> && std::is_object_v<T> && !std::is_array_v<T>;
+
+template<typename T>
+inline constexpr bool is_valid_unexpected_type_v<nonstd::unexpected_type<T>> = false;
Review Comment:
Both specializations refer to a possible `E` argument for `unexpected<E>`, since they're checking if it's a valid unexpected type. The second case just makes sure that `unexpected<unexpected<E>>` isn't accepted.
Shouldn't both of them be changed to `E` instead?
--
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1329755886
##########
libminifi/include/utils/expected.h:
##########
@@ -31,28 +29,42 @@ inline constexpr bool is_expected_v = false;
template<typename V, typename E>
inline constexpr bool is_expected_v<nonstd::expected<V, E>> = true;
-// map implementation
-template<typename Expected, typename F, typename = std::enable_if_t<is_expected_v<utils::remove_cvref_t<Expected>>>>
-auto operator|(Expected&& object, map_wrapper<F> f) {
- using value_type = typename utils::remove_cvref_t<Expected>::value_type;
- using error_type = typename utils::remove_cvref_t<Expected>::error_type;
- if constexpr (std::is_same_v<value_type, void>) {
- using function_return_type = std::decay_t<decltype(std::invoke(std::forward<F>(f.function)))>;
+template<typename T>
+concept expected = is_expected_v<std::remove_cvref_t<T>>;
+
+template<typename T>
+inline constexpr bool is_valid_unexpected_type_v = std::is_same_v<T, std::remove_cvref_t<T>> && std::is_object_v<T> && !std::is_array_v<T>;
+
+template<typename T>
+inline constexpr bool is_valid_unexpected_type_v<nonstd::unexpected_type<T>> = false;
Review Comment:
Both specializations refer to a possible `E` argument for `unexpected<E>`. Shouldn't both of them be changed to `E` instead?
--
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
[GitHub] [nifi-minifi-cpp] fgerlits closed pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits closed pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659
--
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1329751078
##########
libminifi/test/unit/ExpectedTest.cpp:
##########
@@ -484,3 +484,63 @@ TEST_CASE("expected valueOrElse", "[expected][valueOrElse]") {
REQUIRE_THROWS_AS(ex | utils::valueOrElse([](const std::string&) -> int { throw std::exception(); }), std::exception);
REQUIRE_THROWS_AS(std::move(ex) | utils::valueOrElse([](std::string&&) -> int { throw std::exception(); }), std::exception);
}
+
+TEST_CASE("expected transformError", "[expected][transformError]") {
+ auto mul2 = [](int a) { return a * 2; };
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2); // NOLINT(performance-move-const-arg)
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
Review Comment:
I don't mind replacing the mutable rvalue case with this, since it's what is being tested in your example, but I'd still keep the const rvalue case, with the NOLINT exclusion, to keep a full coverage of possible input reference types.
--
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1329751078
##########
libminifi/test/unit/ExpectedTest.cpp:
##########
@@ -484,3 +484,63 @@ TEST_CASE("expected valueOrElse", "[expected][valueOrElse]") {
REQUIRE_THROWS_AS(ex | utils::valueOrElse([](const std::string&) -> int { throw std::exception(); }), std::exception);
REQUIRE_THROWS_AS(std::move(ex) | utils::valueOrElse([](std::string&&) -> int { throw std::exception(); }), std::exception);
}
+
+TEST_CASE("expected transformError", "[expected][transformError]") {
+ auto mul2 = [](int a) { return a * 2; };
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2); // NOLINT(performance-move-const-arg)
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
Review Comment:
I don't mind replacing the mutable rvalue case with this, since it's what is being tested in your example, but I'd still keep the const rvalue case, with the NOLINT exclusion, to keep a full coverage of possible input reference types.
What do you think about replacing one or both mutable rvalue reference blocks with ones similar to your example, while keeping the `const expected&&` blocks that clang-tidy warns about?
--
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1330273703
##########
libminifi/test/unit/ExpectedTest.cpp:
##########
@@ -153,124 +153,124 @@ TEST_CASE("expected map", "[expected][map]") {
// mapping functions which return references
{
nonstd::expected<int, int> e(42);
- auto ret = e | utils::map([](int& i) -> int& { return i; });
+ auto ret = e | utils::transform([](int& i) -> int& { return i; });
REQUIRE(ret);
REQUIRE(ret == 42);
}
}
-TEST_CASE("expected flatMap", "[expected][flatMap]") {
+TEST_CASE("expected andThen", "[expected][andThen]") {
auto succeed = [](int) { return nonstd::expected<int, int>(21 * 2); };
auto fail = [](int) { return nonstd::expected<int, int>(nonstd::unexpect, 17); };
{
nonstd::expected<int, int> e = 21;
- auto ret = e | utils::flatMap(succeed);
+ auto ret = e | utils::andThen(succeed);
REQUIRE(ret);
REQUIRE(*ret == 42);
}
{
const nonstd::expected<int, int> e = 21;
- auto ret = e | utils::flatMap(succeed);
+ auto ret = e | utils::andThen(succeed);
REQUIRE(ret);
REQUIRE(*ret == 42);
}
{
nonstd::expected<int, int> e = 21;
- auto ret = std::move(e) | utils::flatMap(succeed);
+ auto ret = std::move(e) | utils::andThen(succeed);
REQUIRE(ret);
REQUIRE(*ret == 42);
}
{
const nonstd::expected<int, int> e = 21;
- auto ret = std::move(e) | utils::flatMap(succeed); // NOLINT(performance-move-const-arg)
+ auto ret = std::move(e) | utils::andThen(succeed); // NOLINT(performance-move-const-arg)
REQUIRE(ret);
REQUIRE(*ret == 42);
}
{
nonstd::expected<int, int> e = 21;
- auto ret = e | utils::flatMap(fail);
+ auto ret = e | utils::andThen(fail);
REQUIRE(!ret);
REQUIRE(ret.error() == 17);
}
{
const nonstd::expected<int, int> e = 21;
- auto ret = e | utils::flatMap(fail);
+ auto ret = e | utils::andThen(fail);
REQUIRE(!ret);
REQUIRE(ret.error() == 17);
}
{
nonstd::expected<int, int> e = 21;
- auto ret = std::move(e) | utils::flatMap(fail);
+ auto ret = std::move(e) | utils::andThen(fail);
REQUIRE(!ret);
REQUIRE(ret.error() == 17);
}
{
const nonstd::expected<int, int> e = 21;
- auto ret = std::move(e) | utils::flatMap(fail); // NOLINT(performance-move-const-arg)
+ auto ret = std::move(e) | utils::andThen(fail); // NOLINT(performance-move-const-arg)
REQUIRE(!ret);
REQUIRE(ret.error() == 17);
}
{
nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = e | utils::flatMap(succeed);
+ auto ret = e | utils::andThen(succeed);
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
const nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = e | utils::flatMap(succeed);
+ auto ret = e | utils::andThen(succeed);
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = std::move(e) | utils::flatMap(succeed);
+ auto ret = std::move(e) | utils::andThen(succeed);
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
const nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = std::move(e) | utils::flatMap(succeed); // NOLINT(performance-move-const-arg)
+ auto ret = std::move(e) | utils::andThen(succeed); // NOLINT(performance-move-const-arg)
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = e | utils::flatMap(fail);
+ auto ret = e | utils::andThen(fail);
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
const nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = e | utils::flatMap(fail);
+ auto ret = e | utils::andThen(fail);
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = std::move(e) | utils::flatMap(fail);
+ auto ret = std::move(e) | utils::andThen(fail);
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
const nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = std::move(e) | utils::flatMap(fail); // NOLINT(performance-move-const-arg)
+ auto ret = std::move(e) | utils::andThen(fail); // NOLINT(performance-move-const-arg)
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
Review Comment:
Thanks for pointing this out, reenabled them in [47aede1](https://github.com/apache/nifi-minifi-cpp/pull/1659/commits/47aede10d43a4b9e0c0b27b67dda824bd5748096)
--
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1327897415
##########
CPPLINT.cfg:
##########
@@ -1,2 +1,2 @@
set noparent
-filter=-runtime/reference,-runtime/string,-build/c++11,-build/include_subdir,-whitespace/forcolon,-build/namespaces_literals,-readability/check
+filter=-runtime/reference,-runtime/string,-build/c++11,-build/include_subdir,-whitespace/forcolon,-build/namespaces_literals,-readability/check,-build/include_what_you_use
Review Comment:
`build/include_what_you_use` produces many false positives: it doesn't check the namespace, just the name of the function. I ran into another example in #1648
```
szaszm@szaszm-pc:~/nifi-minifi-cpp-3/build-gcc-make$ make linter
Built target minifi-usb-camera-extensions-linter
/home/szaszm/nifi-minifi-cpp-3/libminifi/include/utils/detail/MonadicOperationWrappers.h:61: Add #include <algorithm> for transform [build/include_what_you_use] [4]
Done processing /home/szaszm/nifi-minifi-cpp-3/libminifi/include/utils/detail/MonadicOperationWrappers.h
/home/szaszm/nifi-minifi-cpp-3/libminifi/include/DiskSpaceWatchdog.h:33: At least two spaces is best between code and comments [whitespace/comments] [2]
/home/szaszm/nifi-minifi-cpp-3/libminifi/include/DiskSpaceWatchdog.h:52: At least two spaces is best between code and comments [whitespace/comments] [2]
Done processing /home/szaszm/nifi-minifi-cpp-3/libminifi/include/DiskSpaceWatchdog.h
Total errors found: 1
/home/szaszm/nifi-minifi-cpp-3/libminifi/include/properties/Decryptor.h:46: Add #include <algorithm> for transform [build/include_what_you_use] [4]
Done processing /home/szaszm/nifi-minifi-cpp-3/libminifi/include/properties/Decryptor.h
/home/szaszm/nifi-minifi-cpp-3/libminifi/include/core/logging/LoggerConfiguration.h:65: Add #include <algorithm> for transform [build/include_what_you_use] [4]
Done processing /home/szaszm/nifi-minifi-cpp-3/libminifi/include/core/logging/LoggerConfiguration.h
```
--
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1329595643
##########
libminifi/include/utils/expected.h:
##########
@@ -61,56 +73,54 @@ auto operator|(Expected&& object, map_wrapper<F> f) {
}
}
-// flatMap
-template<typename Expected, typename F, typename = std::enable_if_t<is_expected_v<utils::remove_cvref_t<Expected>>>>
-auto operator|(Expected&& object, flat_map_wrapper<F> f) {
- using value_type = typename utils::remove_cvref_t<Expected>::value_type;
- if constexpr (std::is_same_v<value_type, void>) {
- using function_return_type = std::decay_t<decltype(std::invoke(std::forward<F>(f.function)))>;
+// andThen
+template<expected Expected, typename F>
+auto operator|(Expected&& object, and_then_wrapper<F> f) {
+ using value_type = typename std::remove_cvref_t<Expected>::value_type;
+ if constexpr (!std::is_void_v<value_type>) {
Review Comment:
No specific reason, I just had to rewrite the void support after attempting to remove it, and realizing that it's still needed, and it just happened to be in this order. I can flip it if it makes things clearer.
--
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1329686011
##########
libminifi/include/utils/expected.h:
##########
@@ -61,56 +73,54 @@ auto operator|(Expected&& object, map_wrapper<F> f) {
}
}
-// flatMap
-template<typename Expected, typename F, typename = std::enable_if_t<is_expected_v<utils::remove_cvref_t<Expected>>>>
-auto operator|(Expected&& object, flat_map_wrapper<F> f) {
- using value_type = typename utils::remove_cvref_t<Expected>::value_type;
- if constexpr (std::is_same_v<value_type, void>) {
- using function_return_type = std::decay_t<decltype(std::invoke(std::forward<F>(f.function)))>;
+// andThen
+template<expected Expected, typename F>
+auto operator|(Expected&& object, and_then_wrapper<F> f) {
+ using value_type = typename std::remove_cvref_t<Expected>::value_type;
+ if constexpr (!std::is_void_v<value_type>) {
Review Comment:
Yes, I think using the same order everywhere would make things clearer.
--
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1328928477
##########
libminifi/include/utils/expected.h:
##########
@@ -31,28 +29,42 @@ inline constexpr bool is_expected_v = false;
template<typename V, typename E>
inline constexpr bool is_expected_v<nonstd::expected<V, E>> = true;
-// map implementation
-template<typename Expected, typename F, typename = std::enable_if_t<is_expected_v<utils::remove_cvref_t<Expected>>>>
-auto operator|(Expected&& object, map_wrapper<F> f) {
- using value_type = typename utils::remove_cvref_t<Expected>::value_type;
- using error_type = typename utils::remove_cvref_t<Expected>::error_type;
- if constexpr (std::is_same_v<value_type, void>) {
- using function_return_type = std::decay_t<decltype(std::invoke(std::forward<F>(f.function)))>;
+template<typename T>
+concept expected = is_expected_v<std::remove_cvref_t<T>>;
+
+template<typename T>
+inline constexpr bool is_valid_unexpected_type_v = std::is_same_v<T, std::remove_cvref_t<T>> && std::is_object_v<T> && !std::is_array_v<T>;
+
+template<typename T>
+inline constexpr bool is_valid_unexpected_type_v<nonstd::unexpected_type<T>> = false;
Review Comment:
This `T` is different from the `T` parameters in lines 35 and 41. Can we rename it to something else, eg. `E`, please?
##########
libminifi/test/unit/ExpectedTest.cpp:
##########
@@ -484,3 +484,63 @@ TEST_CASE("expected valueOrElse", "[expected][valueOrElse]") {
REQUIRE_THROWS_AS(ex | utils::valueOrElse([](const std::string&) -> int { throw std::exception(); }), std::exception);
REQUIRE_THROWS_AS(std::move(ex) | utils::valueOrElse([](std::string&&) -> int { throw std::exception(); }), std::exception);
}
+
+TEST_CASE("expected transformError", "[expected][transformError]") {
+ auto mul2 = [](int a) { return a * 2; };
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2); // NOLINT(performance-move-const-arg)
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
Review Comment:
I think `clang-tidy` has a point here: this test doesn't make much sense, since we can't move from a `const` variable. I would remove this test case, and the others marked with `NOLINT(performance-move-const-arg)`, too. Maybe they could be replaced with more realistic tests where the `expected` comes from the return value of a function.
##########
libminifi/include/utils/expected.h:
##########
@@ -61,56 +73,54 @@ auto operator|(Expected&& object, map_wrapper<F> f) {
}
}
-// flatMap
-template<typename Expected, typename F, typename = std::enable_if_t<is_expected_v<utils::remove_cvref_t<Expected>>>>
-auto operator|(Expected&& object, flat_map_wrapper<F> f) {
- using value_type = typename utils::remove_cvref_t<Expected>::value_type;
- if constexpr (std::is_same_v<value_type, void>) {
- using function_return_type = std::decay_t<decltype(std::invoke(std::forward<F>(f.function)))>;
+// andThen
+template<expected Expected, typename F>
+auto operator|(Expected&& object, and_then_wrapper<F> f) {
+ using value_type = typename std::remove_cvref_t<Expected>::value_type;
+ if constexpr (!std::is_void_v<value_type>) {
Review Comment:
Why did you flip this? Before, we always dealt with the `void` case first and the non-`void` case second. Now, it is the other way round in this one instance.
--
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1329693792
##########
libminifi/test/unit/ExpectedTest.cpp:
##########
@@ -484,3 +484,63 @@ TEST_CASE("expected valueOrElse", "[expected][valueOrElse]") {
REQUIRE_THROWS_AS(ex | utils::valueOrElse([](const std::string&) -> int { throw std::exception(); }), std::exception);
REQUIRE_THROWS_AS(std::move(ex) | utils::valueOrElse([](std::string&&) -> int { throw std::exception(); }), std::exception);
}
+
+TEST_CASE("expected transformError", "[expected][transformError]") {
+ auto mul2 = [](int a) { return a * 2; };
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2); // NOLINT(performance-move-const-arg)
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
Review Comment:
As it is, this is the same test as the one at lines 499-502, just with a confusing `move` added which doesn't do anything.
I think something like this would be slightly different from lines 506-509:
```c++
{
const auto expected_provider = [] { return = nonstd::expected<int, int>(nonstd::unexpected, 21); };
auto ret = expected_provider() | utils::transformError(mul2);
REQUIRE(!ret);
REQUIRE(ret.error() == 42);
}
```
but if you don't think it's different enough, I'm OK with removing this test case.
--
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1329693792
##########
libminifi/test/unit/ExpectedTest.cpp:
##########
@@ -484,3 +484,63 @@ TEST_CASE("expected valueOrElse", "[expected][valueOrElse]") {
REQUIRE_THROWS_AS(ex | utils::valueOrElse([](const std::string&) -> int { throw std::exception(); }), std::exception);
REQUIRE_THROWS_AS(std::move(ex) | utils::valueOrElse([](std::string&&) -> int { throw std::exception(); }), std::exception);
}
+
+TEST_CASE("expected transformError", "[expected][transformError]") {
+ auto mul2 = [](int a) { return a * 2; };
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2); // NOLINT(performance-move-const-arg)
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
Review Comment:
As it is, this is the same test as the one at lines 499-502, just with a confusing `move` added which doesn't do anything.
I think something like this would be slightly different from lines 506-509:
```c++
{
const auto expected_provider = [] { return nonstd::expected<int, int>(nonstd::unexpected, 21); };
auto ret = expected_provider() | utils::transformError(mul2);
REQUIRE(!ret);
REQUIRE(ret.error() == 42);
}
```
but if you don't think it's different enough, I'm OK with removing this test case.
--
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1329594537
##########
libminifi/include/utils/expected.h:
##########
@@ -31,28 +29,42 @@ inline constexpr bool is_expected_v = false;
template<typename V, typename E>
inline constexpr bool is_expected_v<nonstd::expected<V, E>> = true;
-// map implementation
-template<typename Expected, typename F, typename = std::enable_if_t<is_expected_v<utils::remove_cvref_t<Expected>>>>
-auto operator|(Expected&& object, map_wrapper<F> f) {
- using value_type = typename utils::remove_cvref_t<Expected>::value_type;
- using error_type = typename utils::remove_cvref_t<Expected>::error_type;
- if constexpr (std::is_same_v<value_type, void>) {
- using function_return_type = std::decay_t<decltype(std::invoke(std::forward<F>(f.function)))>;
+template<typename T>
+concept expected = is_expected_v<std::remove_cvref_t<T>>;
+
+template<typename T>
+inline constexpr bool is_valid_unexpected_type_v = std::is_same_v<T, std::remove_cvref_t<T>> && std::is_object_v<T> && !std::is_array_v<T>;
+
+template<typename T>
+inline constexpr bool is_valid_unexpected_type_v<nonstd::unexpected_type<T>> = false;
Review Comment:
I can rename it, but I don't understand the problem: it's a generic name used for type parameters when communicating more context isn't necessary. Is it really confusing to have identically named `typename T` in a partial template specialization and a related concept definition?
Would it be better to use separate names for all of them (let's say `T`, `U` and `V`), or some other arrangement (like `T` for the first one, `E` for the partial specialization, `F` for the concept)?
--
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1329751078
##########
libminifi/test/unit/ExpectedTest.cpp:
##########
@@ -484,3 +484,63 @@ TEST_CASE("expected valueOrElse", "[expected][valueOrElse]") {
REQUIRE_THROWS_AS(ex | utils::valueOrElse([](const std::string&) -> int { throw std::exception(); }), std::exception);
REQUIRE_THROWS_AS(std::move(ex) | utils::valueOrElse([](std::string&&) -> int { throw std::exception(); }), std::exception);
}
+
+TEST_CASE("expected transformError", "[expected][transformError]") {
+ auto mul2 = [](int a) { return a * 2; };
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = e | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2);
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
+
+ {
+ const nonstd::expected<int, int> e = nonstd::make_unexpected(21);
+ auto ret = std::move(e) | utils::transformError(mul2); // NOLINT(performance-move-const-arg)
+ REQUIRE(!ret);
+ REQUIRE(ret.error() == 42);
+ }
Review Comment:
I don't mind replacing the mutable rvalue case with this, since it's what is being tested in your example, but I'd still keep the const rvalue case, with the NOLINT exclusion, to keep a full coverage of possible input reference types.
What do you think about replacing one or both mutable rvalue reference blocks with ones similar to your example?
--
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
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1659: MINIFICPP-2218 Refactor expected monadic functions
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1659:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1659#discussion_r1330043347
##########
libminifi/test/unit/ExpectedTest.cpp:
##########
@@ -153,124 +153,124 @@ TEST_CASE("expected map", "[expected][map]") {
// mapping functions which return references
{
nonstd::expected<int, int> e(42);
- auto ret = e | utils::map([](int& i) -> int& { return i; });
+ auto ret = e | utils::transform([](int& i) -> int& { return i; });
REQUIRE(ret);
REQUIRE(ret == 42);
}
}
-TEST_CASE("expected flatMap", "[expected][flatMap]") {
+TEST_CASE("expected andThen", "[expected][andThen]") {
auto succeed = [](int) { return nonstd::expected<int, int>(21 * 2); };
auto fail = [](int) { return nonstd::expected<int, int>(nonstd::unexpect, 17); };
{
nonstd::expected<int, int> e = 21;
- auto ret = e | utils::flatMap(succeed);
+ auto ret = e | utils::andThen(succeed);
REQUIRE(ret);
REQUIRE(*ret == 42);
}
{
const nonstd::expected<int, int> e = 21;
- auto ret = e | utils::flatMap(succeed);
+ auto ret = e | utils::andThen(succeed);
REQUIRE(ret);
REQUIRE(*ret == 42);
}
{
nonstd::expected<int, int> e = 21;
- auto ret = std::move(e) | utils::flatMap(succeed);
+ auto ret = std::move(e) | utils::andThen(succeed);
REQUIRE(ret);
REQUIRE(*ret == 42);
}
{
const nonstd::expected<int, int> e = 21;
- auto ret = std::move(e) | utils::flatMap(succeed); // NOLINT(performance-move-const-arg)
+ auto ret = std::move(e) | utils::andThen(succeed); // NOLINT(performance-move-const-arg)
REQUIRE(ret);
REQUIRE(*ret == 42);
}
{
nonstd::expected<int, int> e = 21;
- auto ret = e | utils::flatMap(fail);
+ auto ret = e | utils::andThen(fail);
REQUIRE(!ret);
REQUIRE(ret.error() == 17);
}
{
const nonstd::expected<int, int> e = 21;
- auto ret = e | utils::flatMap(fail);
+ auto ret = e | utils::andThen(fail);
REQUIRE(!ret);
REQUIRE(ret.error() == 17);
}
{
nonstd::expected<int, int> e = 21;
- auto ret = std::move(e) | utils::flatMap(fail);
+ auto ret = std::move(e) | utils::andThen(fail);
REQUIRE(!ret);
REQUIRE(ret.error() == 17);
}
{
const nonstd::expected<int, int> e = 21;
- auto ret = std::move(e) | utils::flatMap(fail); // NOLINT(performance-move-const-arg)
+ auto ret = std::move(e) | utils::andThen(fail); // NOLINT(performance-move-const-arg)
REQUIRE(!ret);
REQUIRE(ret.error() == 17);
}
{
nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = e | utils::flatMap(succeed);
+ auto ret = e | utils::andThen(succeed);
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
const nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = e | utils::flatMap(succeed);
+ auto ret = e | utils::andThen(succeed);
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = std::move(e) | utils::flatMap(succeed);
+ auto ret = std::move(e) | utils::andThen(succeed);
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
const nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = std::move(e) | utils::flatMap(succeed); // NOLINT(performance-move-const-arg)
+ auto ret = std::move(e) | utils::andThen(succeed); // NOLINT(performance-move-const-arg)
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = e | utils::flatMap(fail);
+ auto ret = e | utils::andThen(fail);
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
const nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = e | utils::flatMap(fail);
+ auto ret = e | utils::andThen(fail);
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = std::move(e) | utils::flatMap(fail);
+ auto ret = std::move(e) | utils::andThen(fail);
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
{
const nonstd::expected<int, int> e(nonstd::unexpect, 21);
- auto ret = std::move(e) | utils::flatMap(fail); // NOLINT(performance-move-const-arg)
+ auto ret = std::move(e) | utils::andThen(fail); // NOLINT(performance-move-const-arg)
REQUIRE(!ret);
REQUIRE(ret.error() == 21);
}
Review Comment:
There is a TEST_CASE here with a comment
`// commented out parts depend on https://github.com/martinmoene/expected-lite/pull/40 (waiting for release)`
I think we upgraded to the release which it dependend upon. Could you enable those test cases aswell?
--
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