[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1377: MINIFICPP-1865 Integrate performance clang-tidy checks in CI

[GitBox <gi...@apache.org>]

szaszm commented on code in PR #1377:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1377#discussion_r941415079


##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -739,7 +739,7 @@ utils::TaskRescheduleInfo C2Agent::produce() {
         std::make_move_iterator(payload_batch.end()),
         [&] (C2Payload&& payload) {
           try {
-            C2Payload && response = protocol_.load()->consumePayload(std::move(payload));
+            C2Payload && response = protocol_.load()->consumePayload(payload);

Review Comment:
   Could you change `response` from reference to variable? I find it confusing whenever code unnecessarily relies on temporary lifetime extension, when it could just normally store the data on the stack.



##########
libminifi/test/unit/ExpectedTest.cpp:
##########
@@ -52,7 +52,7 @@ TEST_CASE("expected map", "[expected][map]") {
 
   {
     const nonstd::expected<int, int> e = 21;
-    auto ret = std::move(e) | utils::map(mul2);
+    auto ret = e | utils::map(mul2);

Review Comment:
   These are meant to test utilities on rvalues. You can see the same tests above on lvalues. Please revert these changes, except for the last one.



##########
libminifi/test/unit/IdTests.cpp:
##########
@@ -77,7 +77,7 @@ TEST_CASE("Test Generate Move", "[id]") {
 
   auto generated = generator->generate();
   auto str = generated.to_string();
-  utils::Identifier moved = std::move(generated);
+  utils::Identifier moved = generated;

Review Comment:
   This change is also removing an intentional move.



-- 
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