[GitHub] [arrow] westonpace commented on a diff in pull request #34777: MINOR: [C++] Remove std::move when calling ProcessEmit and ProcessExtensionEmit

["westonpace (via GitHub)" <gi...@apache.org>]

westonpace commented on code in PR #34777:
URL: https://github.com/apache/arrow/pull/34777#discussion_r1154711477


##########
cpp/src/arrow/engine/substrait/relation_internal.cc:
##########
@@ -89,7 +89,7 @@ Result<EmitInfo> GetEmitInfo(const RelMessage& rel,
   }
   emit_info.expressions = std::move(proj_field_refs);
   emit_info.schema = schema(std::move(emit_fields));
-  return std::move(emit_info);

Review Comment:
   I recall that one of the older compilers (I think mingw) does not recognize that `emit_info` is the return value because the return type is `Result<EmitInfo>` and the type of `emit_info` is `EmitInfo`.  See, for example, https://stackoverflow.com/questions/74459462/why-c-does-not-perform-rvo-to-stdoptional .  However, maybe it was one of the older gcc compilers that we no longer support now that we upgraded to C++17.
   
   We should detect it, if it is a problem, if the nightly tests pass.  However, the existing convention in the code base is still to use `std::move` when the return type is `Result<T>` and the object is `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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org