GH-41367: [C++] Replace [[maybe_unused]] with Arrow macro #41359

WillAyd · 2024-04-23T22:56:45Z

Rationale for this change

This is a follow up to #41111 which was created as an issue in #41367

What changes are included in this PR?

Replace [[maybe_unused]] with Arrow macro

Are these changes tested?

Builds cleanly

Are there any user-facing changes?

No

GitHub Issue: [C++] [[maybe_unused]] attribute does not work with g++ 8.5.0 builds #41367

kou · 2024-04-23T23:35:05Z

Could you open a new issue for this? In general, we use one issue per PR.

kou · 2024-04-23T23:35:39Z

@github-actions crossbow submit almalinux-8-amd64

github-actions · 2024-04-23T23:38:10Z

Revision: 9abc710

Submitted crossbow builds: ursacomputing/crossbow @ actions-4f5b08fc0b

Task	Status
almalinux-8-amd64

github-actions · 2024-04-24T16:24:09Z

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

WillAyd · 2024-04-24T19:17:05Z

As far as I can tell none of the current CI failures are related to this change

kou · 2024-04-24T20:55:46Z

The "AMD64 macOS 12 GLib & Ruby" failure: #41369

kou · 2024-04-24T21:00:39Z

The "AMD64 macOS 12 MATLAB" failure: #41370

kou · 2024-04-24T21:03:23Z

The "Source Release and Merge Script on macos-latest" failure: #41371

kou · 2024-04-24T21:10:02Z

cpp/src/arrow/device.h

+  virtual Result<std::shared_ptr<Stream>> WrapStream(void* device_stream,
+                                                     Stream::release_fn_t release_fn) {
+    ARROW_UNUSED(device_stream);
+    ARROW_UNUSED(release_fn);


Can we use ARROW_ARG_UNUSED() by adding ARROW_ARG_UNUSED(x)=x to Doxygen's PREDEFINED?

(ARROW_ARG_UNUSED(x)=x is added and the list is sorted.)

diff --git a/cpp/apidoc/Doxyfile b/cpp/apidoc/Doxyfile index e19c933cd4..5be93032c0 100644 --- a/cpp/apidoc/Doxyfile +++ b/cpp/apidoc/Doxyfile @@ -2168,16 +2168,17 @@ INCLUDE_FILE_PATTERNS = PREDEFINED = __attribute__(x)= \ __declspec(x)= \ - PARQUET_EXPORT= \ - GANDIVA_EXPORT= \ - ARROW_EXPORT= \ ARROW_ACERO_EXPORT= \ + ARROW_ARG_UNUSED(x)=x \ + ARROW_DEPRECATED(x)= \ ARROW_DS_EXPORT= \ ARROW_ENGINE_EXPORT= \ + ARROW_EXPORT= \ + ARROW_EXTERN_TEMPLATE= \ ARROW_FLIGHT_EXPORT= \ ARROW_FLIGHT_SQL_EXPORT= \ - ARROW_EXTERN_TEMPLATE= \ - ARROW_DEPRECATED(x)= + GANDIVA_EXPORT= \ + PARQUET_EXPORT= # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this # tag can be used to specify a list of macro names that should be expanded. The

See also: https://www.doxygen.nl/manual/preprocessing.html

I can get this to work with smaller examples but no such luck yet with the Arrow build. Need to investigate further

After some more digging it seems like what you suggested works for Doxygen to generate the appropriate output, but that seems to have no bearing on clang. I think this issue upstream is related llvm/llvm-project#77489

Here's an MRE.

test.cc:

#define ARG_UNUSED(x) /// @brief A function /// /// @param foo - this is not really used void SomeFunction(void ARG_UNUSED(foo)) { return; }

Doxyfile:

PROJECT_NAME = foo INPUT = test.cc MACRO_EXPANSION = YES PREDEFINED = ARG_UNUSED(x)=x

doxygen -d Preprocessor output:

Doxygen version used: 1.9.1 Searching for include files... Searching for example files... Searching for images... Searching for dot files... Searching for msc files... Searching for dia files... Searching for files to exclude Searching INPUT for files to process... Reading and parsing tag files Parsing files Preprocessing /home/willayd/Desktop/test.cc... Preprocessor output of /home/willayd/Desktop/test.cc (size: 134 bytes): --------- 00001 #define ARG_UNUSED(x) 00002 00003 /// @brief A function 00004 /// 00005 /// @param foo - this is not really used 00006 void SomeFunction(void foo ) { 00007 return; 00008 } 00009 --------- Macros accessible in this file (/home/willayd/Desktop/test.cc): --------- ARG_UNUSED ---------

clang++ test.cc -Wdocumentation -shared output:

test.cc:5:12: warning: parameter 'foo' not found in the function declaration [-Wdocumentation] /// @param foo - this is not really used ^~~ 1 warning generated.

Oh... How about this?

diff --git a/cpp/cmake_modules/SetupCxxFlags.cmake b/cpp/cmake_modules/SetupCxxFlags.cmake index d56609c123..ea357b4779 100644 --- a/cpp/cmake_modules/SetupCxxFlags.cmake +++ b/cpp/cmake_modules/SetupCxxFlags.cmake @@ -314,6 +314,7 @@ if("${BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN") set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wall") set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wextra") set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wdocumentation") + set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -DARROW_WARN_DOCUMENTATION") if(CMAKE_SYSTEM_NAME STREQUAL "Emscripten") # size_t is 32 bit in Emscripten wasm32 - ignore conversion errors set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-shorten-64-to-32") diff --git a/cpp/src/arrow/util/macros.h b/cpp/src/arrow/util/macros.h index d80828869b..6faf25b3c9 100644 --- a/cpp/src/arrow/util/macros.h +++ b/cpp/src/arrow/util/macros.h @@ -67,7 +67,11 @@ // [5] J. Doerfert et al. 2019. "Performance Exploration Through Optimistic Static // Program Annotations". https://github.com/jdoerfert/PETOSPA/blob/master/ISC19.pdf #define ARROW_UNUSED(x) (void)(x) +#ifdef ARROW_WARN_DOCUMENTATION +#define ARROW_ARG_UNUSED(x) x +#else #define ARROW_ARG_UNUSED(x) +#endif #if defined(__GNUC__) // GCC and compatible compilers (clang, Intel ICC) #define ARROW_NORETURN __attribute__((noreturn)) #define ARROW_NOINLINE __attribute__((noinline))

Just pushed up this change

This reverts commit 84cc37e.

kevingurney · 2024-04-25T19:22:53Z

The "AMD64 macOS 12 MATLAB" failure: #41370

FYI - this issue has been addressed in #41384.

raulcd · 2024-04-26T16:14:44Z

@github-actions crossbow submit centos

github-actions · 2024-04-26T16:17:09Z

Revision: dd0c7d4

Submitted crossbow builds: ursacomputing/crossbow @ actions-71b6b957bd

Task	Status
centos-7-amd64
centos-8-stream-amd64
centos-8-stream-arm64
centos-9-stream-amd64
centos-9-stream-arm64
test-r-rstudio-r-base-4.2-centos7-devtoolset-8

This reverts commit dd0c7d4.

kou · 2024-04-27T00:32:50Z

@github-actions crossbow submit preview-docs

github-actions · 2024-04-27T00:35:05Z

Revision: bdf0bff

Submitted crossbow builds: ursacomputing/crossbow @ actions-eb949ffd4d

Task	Status
preview-docs

jonkeane · 2024-04-27T16:32:15Z

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

github-actions · 2024-04-27T16:34:38Z

Revision: bdf0bff

Submitted crossbow builds: ursacomputing/crossbow @ actions-8f6e4a0c9c

Task	Status
test-r-rstudio-r-base-4.1-opensuse153

kou

+1

conbench-apache-arrow · 2024-04-27T23:15:25Z

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

There were no benchmark performance regressions. 🎉

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

…he#41359) ### Rationale for this change This is a follow up to apache#41111 which was created as an issue in apache#41367 ### What changes are included in this PR? Replace [[maybe_unused]] with Arrow macro ### Are these changes tested? Builds cleanly ### Are there any user-facing changes? No * GitHub Issue: apache#41367 Authored-by: Will Ayd <william.ayd@icloud.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

apacheGH-41112: [C++] Replace [[maybe_unused]] with Arrow macro

9abc710

github-actions bot added Component: C++ awaiting review Awaiting review labels Apr 23, 2024

vibhatha requested a review from bkietz April 23, 2024 23:52

ARROW_ARG_UNUSED -> ARROW_UNUSED

f591885

WillAyd changed the title ~~GH-41112: [C++] Replace [[maybe_unused]] with Arrow macro~~ GH-41367: [C++] Replace [[maybe_unused]] with Arrow macro Apr 24, 2024

kou reviewed Apr 24, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Apr 24, 2024

Update Doxygen

f7defe4

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 24, 2024

WillAyd added 2 commits April 24, 2024 18:15

Try EXPAND_AS_DEFINED

84cc37e

Revert "Try EXPAND_AS_DEFINED"

0e888e4

This reverts commit 84cc37e.

Undo doxygen changes

dd0c7d4

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 26, 2024

WillAyd added 2 commits April 26, 2024 12:39

Revert "Undo doxygen changes"

19c3020

This reverts commit dd0c7d4.

Use macro for -Wdocumentation control

bdf0bff

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 26, 2024

assignUser mentioned this pull request Apr 27, 2024

GH-41402: [CI][R] Update our backwards compatibility CI any other R 4.4 cleanups #41403

Merged

kou approved these changes Apr 27, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Apr 27, 2024

kou merged commit 5ee70ee into apache:main Apr 27, 2024

kou removed the awaiting merge Awaiting merge label Apr 27, 2024

kou mentioned this pull request Apr 27, 2024

[C++] [[maybe_unused]] attribute does not work with g++ 8.5.0 builds #41367

Closed

WillAyd deleted the arrow-unused branch April 28, 2024 17:36

GH-41367: [C++] Replace [[maybe_unused]] with Arrow macro #41359

GH-41367: [C++] Replace [[maybe_unused]] with Arrow macro #41359

Uh oh!

Conversation

WillAyd commented Apr 23, 2024 • edited by kou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

kou commented Apr 23, 2024

Uh oh!

kou commented Apr 23, 2024

Uh oh!

github-actions bot commented Apr 23, 2024

Uh oh!

github-actions bot commented Apr 24, 2024

Uh oh!

WillAyd commented Apr 24, 2024

Uh oh!

kou commented Apr 24, 2024

Uh oh!

kou commented Apr 24, 2024

Uh oh!

kou commented Apr 24, 2024

Uh oh!

kou Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

WillAyd Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

WillAyd Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

kevingurney commented Apr 25, 2024

Uh oh!

raulcd commented Apr 26, 2024

Uh oh!

github-actions bot commented Apr 26, 2024

Uh oh!

kou commented Apr 27, 2024

Uh oh!

github-actions bot commented Apr 27, 2024

Uh oh!

jonkeane commented Apr 27, 2024

Uh oh!

github-actions bot commented Apr 27, 2024

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

conbench-apache-arrow bot commented Apr 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WillAyd commented Apr 23, 2024 •

edited by kou

Loading

WillAyd Apr 25, 2024 •

edited

Loading

WillAyd Apr 25, 2024 •

edited

Loading

kou Apr 26, 2024 •

edited

Loading