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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
