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

WillAyd · 2024-05-01T17:50:35Z

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 · 2024-05-01T17:51:01Z

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:

WillAyd · 2024-05-01T17:51:09Z

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 · 2024-05-01T17:52:42Z

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

…-cleanup

kou · 2024-05-05T21:56:59Z

@github-actions crossbow submit -g cpp

github-actions · 2024-05-05T21:59:28Z

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 · 2024-05-05T22:06:02Z

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 · 2024-05-06T20:41:39Z

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 · 2024-05-06T21:18:54Z

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 · 2024-05-07T23:14:03Z

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 · 2024-05-08T00:14:52Z

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)

WillAyd · 2024-05-11T14:31:08Z

Don't have a CUDA device to locally test that. Fixed the latest crossbow issue - hopefully that covers it

kou · 2024-05-11T16:23:08Z

@github-actions crossbow submit -g cpp

github-actions · 2024-05-11T16:25:35Z

Revision: 54cb257

Submitted crossbow builds: ursacomputing/crossbow @ actions-999a090d8e

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

WillAyd · 2024-05-11T18:50:35Z

I don't believe the CI failures are related - maybe flaky tests?

kou · 2024-05-12T06:46:16Z

cpp/src/arrow/dataset/dataset_writer.cc

@@ -409,7 +409,7 @@ class DatasetWriterDirectoryQueue {
    dir_queue->PrepareDirectory();
    ARROW_ASSIGN_OR_RAISE(dir_queue->current_filename_, dir_queue->GetNextFilename());
    // std::move required to make RTools 3.5 mingw compiler happy


Could you remove this comment?

kou · 2024-05-12T06:48:46Z

cpp/src/arrow/io/compressed.cc

    // Using std::move because some compiler might has issue below:
    // https://wg21.cmeerw.net/cwg/issue1579


Hmm. We may not be able to remove this std::move(): #41025

Let's try R jobs.

kou · 2024-05-12T06:49:22Z

cpp/src/arrow/util/iterator.h

-    // ARROW-8193: On gcc-4.8 without the explicit move it tries to use the
-    // copy constructor, which may be deleted on the elements of type T
-    return std::move(out);
+


Suggested change

kou · 2024-05-12T06:49:41Z

@github-actions crossbow submit -g r

kou · 2024-05-12T20:31:51Z

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 · 2024-05-12T21:46:33Z

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

…-cleanup

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 · 2024-05-13T14:01:35Z

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

kou · 2024-05-13T21:23:05Z

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

WillAyd · 2024-05-14T18:48:46Z

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 · 2024-05-15T00:14:53Z

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

github-actions · 2024-05-15T00:17:36Z

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 · 2024-05-15T12:24:28Z

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

GH41478: [C++] Clean up more redundant move warnings

2b1dadd

github-actions bot added the Component: C++ label May 1, 2024

github-actions bot added the awaiting review Awaiting review label May 1, 2024

WillAyd changed the title ~~GH41478: [C++] Clean up more redundant move warnings~~ GH-41478: [C++] Clean up more redundant move warnings May 1, 2024

WillAyd added 3 commits May 2, 2024 09:41

Merge remote-tracking branch 'upstream/main' into more-redundant-move…

1e18c3a

…-cleanup

Merge remote-tracking branch 'upstream/main' into more-redundant-move…

3fc6313

…-cleanup

Merge remote-tracking branch 'upstream/main' into more-redundant-move…

0920f96

…-cleanup

Merge branch 'main' into more-redundant-move-cleanup

57a8d88

WillAyd added 4 commits May 6, 2024 18:02

Try catching error in CI

a848199

Fix warning

cdf234a

More removals

88c6b90

Fix redundant-move warnings

d01c47f

WillAyd requested review from wgtmac and westonpace as code owners May 7, 2024 23:00

github-actions bot added Component: Parquet Component: C++ - Gandiva labels May 7, 2024

fixes

c4639e4

WillAyd requested a review from lidavidm as a code owner May 8, 2024 01:29

github-actions bot added the Component: FlightRPC label May 8, 2024

Fix gandiva issue

6149072

kou reviewed May 12, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels May 12, 2024

This comment was marked as outdated.

Sign in to view

WillAyd added 2 commits May 13, 2024 09:09

Edits

d5fcd96

Merge remote-tracking branch 'upstream/main' into more-redundant-move…

a7f7ad2

…-cleanup

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 13, 2024

westonpace approved these changes May 13, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 13, 2024

This comment was marked as outdated.

Sign in to view

R build compatability

27a162b

This comment was marked as outdated.

Sign in to view

R compat

cc3c73d

kou approved these changes May 15, 2024

View reviewed changes

pitrou approved these changes May 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

WillAyd commented May 1, 2024 •

edited by github-actions bot

github-actions bot commented May 1, 2024

WillAyd commented May 1, 2024

github-actions bot commented May 1, 2024

kou commented May 5, 2024

github-actions bot commented May 5, 2024

kou commented May 5, 2024

WillAyd commented May 6, 2024

kou commented May 6, 2024

WillAyd commented May 7, 2024

kou commented May 8, 2024

WillAyd commented May 11, 2024

kou commented May 11, 2024

github-actions bot commented May 11, 2024

WillAyd commented May 11, 2024

kou May 12, 2024

kou May 12, 2024 •

edited

kou May 12, 2024

kou commented May 12, 2024

This comment was marked as outdated.

kou commented May 12, 2024

kou commented May 12, 2024

westonpace left a comment

kou commented May 13, 2024

This comment was marked as outdated.

kou commented May 13, 2024

This comment was marked as outdated.

WillAyd commented May 14, 2024 •

edited

kou commented May 15, 2024

github-actions bot commented May 15, 2024

WillAyd commented May 15, 2024

kou left a comment

pitrou left a comment

		// Using std::move because some compiler might has issue below:
		// https://wg21.cmeerw.net/cwg/issue1579

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

Are you sure you want to change the base?

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

Conversation

WillAyd commented May 1, 2024 • edited by github-actions bot

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented May 1, 2024

WillAyd commented May 1, 2024

github-actions bot commented May 1, 2024

kou commented May 5, 2024

github-actions bot commented May 5, 2024

kou commented May 5, 2024

WillAyd commented May 6, 2024

kou commented May 6, 2024

WillAyd commented May 7, 2024

kou commented May 8, 2024

WillAyd commented May 11, 2024

kou commented May 11, 2024

github-actions bot commented May 11, 2024

WillAyd commented May 11, 2024

kou May 12, 2024

Choose a reason for hiding this comment

kou May 12, 2024 • edited

Choose a reason for hiding this comment

kou May 12, 2024

Choose a reason for hiding this comment

kou commented May 12, 2024

This comment was marked as outdated.

kou commented May 12, 2024

kou commented May 12, 2024

westonpace left a comment

Choose a reason for hiding this comment

kou commented May 13, 2024

This comment was marked as outdated.

kou commented May 13, 2024

This comment was marked as outdated.

WillAyd commented May 14, 2024 • edited

kou commented May 15, 2024

github-actions bot commented May 15, 2024

WillAyd commented May 15, 2024

kou left a comment

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

WillAyd commented May 1, 2024 •

edited by github-actions bot

kou May 12, 2024 •

edited

WillAyd commented May 14, 2024 •

edited