GH-41478: [C++] Clean up more redundant move warnings

[WillAyd]

Rationale for this change

Minor warning cleanup for downstream libraries trying to get warning-free builds

What changes are included in this PR?

Removed redundant std::move from return statements

Are these changes tested?

Builds cleanly

Are there any user-facing changes?

No

• GitHub Issue: [C++] Clean up Assorted Warnings to get a clean nanoarrow build #41478

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[WillAyd]

Can see this in a branch adding meson warning level 2 downstream to nanoarrow:

https://github.com/apache/arrow-nanoarrow/actions/runs/8912807091/job/24476999593?pr=448

[github-actions]

⚠️ GitHub issue #41478 has been automatically assigned in GitHub to PR creator.

[kou]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: 0920f96

Submitted crossbow builds: ursacomputing/crossbow @ actions-87d0bf1ec5

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14

[kou]

Hmm. Why is -Wredundant-move warning detected in our CI?
It seems that nanoarrow uses warning_level=2 https://github.com/apache/arrow-nanoarrow/blob/ae84d5f1f195f7345fe27d00e09507958746590e/meson.build#L27 . Is it related? If so, should we use -Wextra or -Wredundant-move in our CMake configuration?

[WillAyd]

I think you are asking why redundant move is not is the Arrow CI but correct me if mistaken. Yea Meson's warning level 2 is essentially Wextra/Wall for gcc/clang.

I haven't fully sifted through the current Arrow CMake configuration but I see that there is a BUILD_WARNING_LEVEL for CHECKIN that should pair both Wall and Wextra. I couldn't find that in the workflows - maybe it is not being used?

[kou]

You're right... I missed "not"... Sorry...

Could you try this whether we can detect these warnings in our build?

diff --git a/cpp/cmake_modules/SetupCxxFlags.cmake b/cpp/cmake_modules/SetupCxxFlags.cmake
index ea357b4779..a7001d6774 100644
--- a/cpp/cmake_modules/SetupCxxFlags.cmake
+++ b/cpp/cmake_modules/SetupCxxFlags.cmake
@@ -330,8 +330,9 @@ if("${BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wall")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-conversion")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-sign-conversion")
-    set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wunused-result")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wdate-time")
+    set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wredundant-move")
+    set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wunused-result")
   elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Intel" OR CMAKE_CXX_COMPILER_ID STREQUAL
                                                    "IntelLLVM")
     if(WIN32)

[WillAyd]

So I've added that but it flags a lot of issues in the current code base. What I have so far generates a clean build with the ninja-debug preset but there are still issues; guessing they come from full builds.

I'm happy to clean those up as well just wanted to confirm it isn't too big of a change in its current state with you before continuing.

Another possible issue is that the -Wredundant-move flag also gets applied to non-cpp files, yielding the following warnings during a build:

[140/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/musl/strptime.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[147/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriIp4Base.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[148/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriIp4.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[149/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriMemory.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[150/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriCompare.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[151/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriCommon.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[152/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriNormalizeBase.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[153/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriParseBase.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[154/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriEscape.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[155/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriNormalize.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[156/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriResolve.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[157/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriShorten.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[159/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriFile.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[160/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriRecompose.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[162/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriQuery.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C
[164/780] Building C object src/arrow/CMakeFiles/arrow_vendored.dir/vendored/uriparser/UriParse.c.o
cc1: warning: command-line option ‘-Wredundant-move’ is valid for C++/ObjC++ but not for C

I didn't quite where those flags were getting intermixed yet; something to investigate

[kou]

Ah, sorry. Could you use CXX_ONLY_FLAGS instead?

diff --git a/cpp/cmake_modules/SetupCxxFlags.cmake b/cpp/cmake_modules/SetupCxxFlags.cmake
index ea357b4779..0bfb1e5a29 100644
--- a/cpp/cmake_modules/SetupCxxFlags.cmake
+++ b/cpp/cmake_modules/SetupCxxFlags.cmake
@@ -332,6 +332,7 @@ if("${BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-sign-conversion")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wunused-result")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wdate-time")
+    string(APPEND CXX_ONLY_FLAGS " -Wredundant-move")
   elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Intel" OR CMAKE_CXX_COMPILER_ID STREQUAL
                                                    "IntelLLVM")
     if(WIN32)

[kou]

test-r-rstudio-r-base-4.1-opensuse153 failed:

https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=63897&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=6c939d89-0d1a-51f2-8b30-091a7a82e98c&l=1320

[ 26%] Building CXX object src/arrow/CMakeFiles/arrow_util.dir/util/align_util.cc.o
/arrow/cpp/src/arrow/util/align_util.cc: In function ‘arrow::Result<std::shared_ptr<arrow::Buffer> > arrow::util::EnsureAlignment(std::shared_ptr<arrow::Buffer>, int64_t, arrow::MemoryPool*)’:
/arrow/cpp/src/arrow/util/align_util.cc:162:12: error: could not convert ‘new_buffer’ from ‘std::unique_ptr<arrow::Buffer>’ to ‘arrow::Result<std::shared_ptr<arrow::Buffer> >’
     return new_buffer;
            ^~~~~~~~~~
make[2]: *** [src/arrow/CMakeFiles/arrow_util.dir/build.make:80: src/arrow/CMakeFiles/arrow_util.dir/util/align_util.cc.o] Error 1

Let's update test-r-rstudio-r-base-4.1-opensuse153: #41626

[kou]

Could you rebase on main to use OpenSUSE 15.5 not 15.3?

[westonpace]

+1 (providing CI / nightly tests pass) These look like nice improvements to me. I think the main reason we had the move on return in the first place was due to a compiler bug in the old R 3.5 toolchain. Now that we no longer need to build there I think this is a good cleanup.

[kou]

@github-actions crossbow submit test-r-rstudio-r-base-4.1-opensuse155

[kou]

@github-actions crossbow submit -g cpp -g r

[WillAyd]

Well this is fun...looks like the compilers have conflicting desires. With code like this:

template <typename Options, typename... Properties>
const FunctionOptionsType* GetFunctionOptionsType(const Properties&... properties) {
  ...
  auto options = std::make_unique<Options>();
  RETURN_NOT_OK(
      FromStructScalarImpl<Options>(options.get(), scalar, properties_).status_);

   return std::move(options);
}

The more modern compilers complain about a redundant move:

/home/willayd/clones/arrow/cpp/src/arrow/compute/function_internal.h: In instantiation of ‘arrow::Result<std::unique_ptr<arrow::compute::FunctionOptions> > arrow::compute::internal::GetFunctionOptionsType(const Properties& ...)::OptionsType::FromStructScalar(const arrow::StructScalar&) const [with Options = arrow::compute::ArithmeticOptions; Properties = {arrow::internal::DataMemberProperty<arrow::compute::ArithmeticOptions, bool>}]’:
/home/willayd/clones/arrow/cpp/src/arrow/compute/function_internal.h:697:3:   required from ‘const arrow::compute::FunctionOptionsType* arrow::compute::internal::GetFunctionOptionsType(const Properties& ...) [with Options = arrow::compute::ArithmeticOptions; Properties = {arrow::internal::DataMemberProperty<arrow::compute::ArithmeticOptions, bool>}]’
/home/willayd/clones/arrow/cpp/src/arrow/compute/api_scalar.cc:314:79:   required from here
/home/willayd/clones/arrow/cpp/src/arrow/compute/function_internal.h:687:31: error: redundant move in return statement [-Werror=redundant-move]
  687 |       return std::move(options);

If you remove the std::move(...) then the test-r-rstudio-r-base-4.1-opensuse155 build complains that it cannot convert the templated pointer type to that which matches the function declaration:

  /arrow/cpp/src/arrow/compute/function_internal.h:687:14: error: could not convert ‘options’ from ‘std::unique_ptr<arrow::compute::ArithmeticOptions, std::default_delete<arrow::compute::ArithmeticOptions> >’ to ‘arrow::Result<std::unique_ptr<arrow::compute::FunctionOptions> >’
       return options;

This also seems to happen in cases where the returned value is a unique_ptr but the function/method declaration is that of a shared_ptr; I'm assuming more modern compilers happily convert that automatically for you but the compiler on the openSUSE build does not

For now I've tried to be really explicit about the return type in the offending functions to try and appease all platforms, but happy to change to any better idea

[kou]

@github-actions crossbow submit -g cpp -g r

[github-actions]

Revision: cc3c73d

Submitted crossbow builds: ursacomputing/crossbow @ actions-40bb465e0a

Task	Status
r-binary-packages
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-r-arrow-backwards-compatibility
test-r-clang-sanitizer
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse155
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-r-sanitizer

[WillAyd]

Looks like the valgrind failures are from AWS cryptography code - I don't think that is related to this

[kou]

+1

@pitrou @bkietz Do you want to review this before we merge this?

[pitrou]

+1, thanks for doing this

[bkietz]

Thanks for doing this!

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 3a4fcff.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.