GH-41112: [C++] Clean up unused parameter warnings

[WillAyd]

Noticed these when compiling the nanoarrow test suite with -Wextra

• GitHub Issue: [C++] Unused parameter warnings when compiling from nanoarrow #41112

Rationale for this change

Helps clean up build warnings when compiling with -Wextra

What changes are included in this PR?

Used Arrow macros to mark some parameters as unused

Are these changes tested?

N/A - just ensured program compiles cleanly

Are there any user-facing changes?

No

[felipecrv]

I hate to be the one bringing boring news, but if a change touches code it can't be considered MINOR: , so please create an issue. The change looks great! Thank you for using ARROW_UNUSED instead of simply removing the parameter names from the function definitions.

[WillAyd]

Good to know and not a problem at all. See #41112

[kou]

Please don't mix unrelated changes.
These changes are for #41107 , right?

[WillAyd]

Ah yes sorry about that. Was working off of one branch to try and get a clean nanoarrow build so this slipped in. Will revert

[github-actions]

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

[kou]

Could you use our PR template instead of removing it entirely next time?

[kou]

How about using ARROW_ARG_UNUSED() instead?

Suggested change

	virtual Status AppendArraySlice(const ArraySpan& array, int64_t offset,
	int64_t length) {
	ARROW_UNUSED(array);
	ARROW_UNUSED(offset);
	ARROW_UNUSED(length);
	virtual Status AppendArraySlice(const ArraySpan& ARROW_ARG_UNUSED(array), int64_t ARROW_ARG_UNUSED(offset),
	int64_t ARROW_ARG_UNUSED(length)) {

[WillAyd]

Sure that works. Out of curiosity now that we are on C++17 any reason not to use [[maybe_unused]]?

[kou]

I think that we can use [[maybe_unused]].
Could you open a new issue for this? Let's discuss this on the issue.

[pitrou]

Can you just try to use [[maybe_unused]] in this PR?

[pitrou]

Hmm, is this necessary? This is making the source code more annoying to read without any tangible benefit.

(we would be bound to add such annotations everywhere a stub function implementation returns NotImplementedError, AFAIU)

[WillAyd]

Its a fair question. It looks like gcc adds -Wunused-parameter only when both Wextra and Wall are enabled.

I have no strong opinion either way - if we think this is too much noise in the code then I don't think its the end of the world for downstream libraries to have to ignore this warning

[felipecrv]

Hmm, is this necessary? This is making the source code more annoying to read without any tangible benefit.

(we would be bound to add such annotations everywhere a stub function implementation returns NotImplementedError, AFAIU)

Then we should add -Wno-unused-parameter to the builds. Otherwise people that use GCC will keep sending these PRs @pitrou.

[pitrou]

@felipecrv This happens in the Arrow headers, so changing our own build flags wouldn't change anything here, AFAICT.

That said, if other people think these warnings do need to be fixed, then why not.

[felipecrv]

+1

That said, if other people think these warnings do need to be fixed, then why not.

Yes, we should help people that build their own software with -Werror.

[conbench-apache-arrow]

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

There were 5 benchmark results indicating a performance regression:

• Commit Run on ursa-i9-9960x at 2024-04-18 20:10:10Z
  • tpch (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-01, scale_factor=1
  • tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-02, scale_factor=1
• and 3 more (see the report linked below)

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

[kou]

Hmm.
It seems that [[maybe_unused]] doesn't work with g++ 8.5.0:

https://github.com/ursacomputing/crossbow/actions/runs/8772764647/job/24072078525#step:6:167

-- The CXX compiler identification is GNU 8.5.0

https://github.com/ursacomputing/crossbow/actions/runs/8772764647/job/24072078525#step:6:1075

FAILED: src/arrow/CMakeFiles/arrow_array.dir/array/array_run_end.cc.o 
/usr/bin/c++ -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_WITH_TIMING_TESTS -I/tmp/arrow-HEAD.P3upj/cpp-build/src -I/arrow/cpp/src -I/arrow/cpp/src/generated -Wno-noexcept-type  -fdiagnostics-color=always  -Wall -fno-semantic-interposition -msse4.2  -O3 -DNDEBUG -O2 -ftree-vectorize  -std=c++17 -fPIC -MD -MT src/arrow/CMakeFiles/arrow_array.dir/array/array_run_end.cc.o -MF src/arrow/CMakeFiles/arrow_array.dir/array/array_run_end.cc.o.d -o src/arrow/CMakeFiles/arrow_array.dir/array/array_run_end.cc.o -c /arrow/cpp/src/arrow/array/array_run_end.cc
In file included from /arrow/cpp/src/arrow/array/array_run_end.cc:19:
/arrow/cpp/src/arrow/array/builder_primitive.h:38:24: error: expected unqualified-id before '[' token
   explicit NullBuilder([[maybe_unused]] const std::shared_ptr<DataType>& type,
                        ^
/arrow/cpp/src/arrow/array/builder_primitive.h:38:24: error: expected ')' before '[' token
   explicit NullBuilder([[maybe_unused]] const std::shared_ptr<DataType>& type,
                       ~^
                        )

[WillAyd]

Unfortunate...so I guess back to the macros?

Unless I am reading this wrong I am surprised by that lack of support - this says gcc 7 started support for maybe_unused?

https://en.cppreference.com/w/cpp/compiler_support/17

[felipecrv]

Unfortunate...so I guess back to the macros?

Yes, please.

[kou]

It seems that [[maybe_unused]] in constructor arguments is only unsupported:

$ docker run -it --rm almalinux:8
$ dnf install -y gcc-c++ vim
$ vim a.cc
$ cat a.cc
class A {
  explicit A([[maybe_unused]] int x) {}
  void a([[maybe_unused]] int x) {}
};
int main([[maybe_unused]] int argc, [[maybe_unused]] char **argv) {
  return 0;
}
$ LANG=C g++ -std=c++17 a.cc
a.cc:2:14: error: expected unqualified-id before '[' token
   explicit A([[maybe_unused]] int x) {}
              ^
a.cc:2:14: error: expected ')' before '[' token
   explicit A([[maybe_unused]] int x) {}
             ~^
              )

[jonkeane]

Thanks for catching and raising that, Kou!

I came here to say that I found similar failures in some of our extended tests for the R implementation (which relies on the C++) implementation and for historical reasons is tested across a bunch of operating systems, including some that while old are still not yet EOL.

Which is causing out nightly builds for test-r-rstudio-r-base-4.1-opensuse153 and test-r-rstudio-r-base-4.2-centos7-devtoolset-8 as well as our r-binary packaging to fail.

[kou]

I've merged WillAyd's fix. Could you try it on your PR?

[jonkeane]

Will do! I also ran one of them on his PR too and it was good there.