Skip to content

Commit c8fe268

Browse files
pitroupitrou
authored
apacheGH-46272: [C++] Build Arrow libraries with -Wmissing-definitions on gcc (apache#47042)
### Rationale for this change The warning option `-Wmissing-declarations` allows finding private functions that erroneously have global linkage (because they are neither static nor in the anonymous namespace). We only care about this for the public Arrow libraries, not for tests or utilities where it's harmless to have private functions that nevertheless have global linkage (and changing that would require a lot of pointless code churn). ### Are these changes tested? Yes, on builds using gcc. ### Are there any user-facing changes? No, because this is only enabled if the warning level is "CHECKIN". Release builds will by default use the "PRODUCTION" warning level. * GitHub Issue: apache#46272 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
1 parent aabd3db commit c8fe268

File tree

119 files changed

+754
-470
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

119 files changed

+754
-470
lines changed

‎cpp/cmake_modules/BuildUtils.cmake‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,7 @@ function(ADD_ARROW_LIB LIB_NAME)
265265
if(ARG_DEFINITIONS)
266266
target_compile_definitions(${LIB_NAME}_objlib PRIVATE ${ARG_DEFINITIONS})
267267
endif()
268+
target_compile_options(${LIB_NAME}_objlib PRIVATE ${ARROW_LIBRARIES_ONLY_CXX_FLAGS})
268269
set(LIB_DEPS $<TARGET_OBJECTS:${LIB_NAME}_objlib>)
269270
set(EXTRA_DEPS)
270271

@@ -326,6 +327,7 @@ function(ADD_ARROW_LIB LIB_NAME)
326327
if(ARG_DEFINITIONS)
327328
target_compile_definitions(${LIB_NAME}_shared PRIVATE ${ARG_DEFINITIONS})
328329
endif()
330+
target_compile_options(${LIB_NAME}_shared PRIVATE ${ARROW_LIBRARIES_ONLY_CXX_FLAGS})
329331

330332
if(ARG_OUTPUTS)
331333
list(APPEND ${ARG_OUTPUTS} ${LIB_NAME}_shared)
@@ -416,6 +418,7 @@ function(ADD_ARROW_LIB LIB_NAME)
416418
if(ARG_DEFINITIONS)
417419
target_compile_definitions(${LIB_NAME}_static PRIVATE ${ARG_DEFINITIONS})
418420
endif()
421+
target_compile_options(${LIB_NAME}_static PRIVATE ${ARROW_LIBRARIES_ONLY_CXX_FLAGS})
419422

420423
if(ARG_OUTPUTS)
421424
list(APPEND ${ARG_OUTPUTS} ${LIB_NAME}_static)

‎cpp/cmake_modules/SetupCxxFlags.cmake‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,9 @@ set(CMAKE_POSITION_INDEPENDENT_CODE ${ARROW_POSITION_INDEPENDENT_CODE})
159159
set(UNKNOWN_COMPILER_MESSAGE
160160
"Unknown compiler: ${CMAKE_CXX_COMPILER_ID} ${CMAKE_CXX_COMPILER_VERSION}")
161161

162+
# Compiler flags used when building Arrow libraries (but not tests, utilities, etc.)
163+
set(ARROW_LIBRARIES_ONLY_CXX_FLAGS)
164+
162165
# compiler flags that are common across debug/release builds
163166
if(WIN32)
164167
# TODO(wesm): Change usages of C runtime functions that MSVC says are
@@ -322,6 +325,9 @@ if("${BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN")
322325
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wimplicit-fallthrough")
323326
string(APPEND CXX_ONLY_FLAGS " -Wredundant-move")
324327
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wunused-result")
328+
# Flag non-static functions that don't have corresponding declaration in a .h file.
329+
# Only for Arrow libraries, since this is not a problem in tests or utilities.
330+
list(APPEND ARROW_LIBRARIES_ONLY_CXX_FLAGS "-Wmissing-declarations")
325331
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Intel" OR CMAKE_CXX_COMPILER_ID STREQUAL
326332
"IntelLLVM")
327333
if(WIN32)

‎cpp/src/arrow/acero/aggregate_internal.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include "arrow/acero/aggregate_internal.h"
2424
#include "arrow/acero/aggregate_node.h"
2525
#include "arrow/acero/exec_plan.h"
26+
#include "arrow/acero/exec_plan_internal.h"
2627
#include "arrow/acero/options.h"
2728
#include "arrow/compute/exec.h"
2829
#include "arrow/compute/function.h"

‎cpp/src/arrow/acero/asof_join_node.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include <unordered_set>
3333

3434
#include "arrow/acero/exec_plan.h"
35+
#include "arrow/acero/exec_plan_internal.h"
3536
#include "arrow/acero/options.h"
3637
#include "arrow/acero/unmaterialized_table_internal.h"
3738
#ifndef NDEBUG

‎cpp/src/arrow/acero/exec_plan.cc‎

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include <unordered_map>
2424
#include <unordered_set>
2525

26+
#include "arrow/acero/exec_plan_internal.h"
2627
#include "arrow/acero/options.h"
2728
#include "arrow/acero/query_context.h"
2829
#include "arrow/acero/task_util.h"
@@ -1101,23 +1102,6 @@ Result<std::unique_ptr<RecordBatchReader>> DeclarationToReader(
11011102
return DeclarationToReader(std::move(declaration), std::move(options));
11021103
}
11031104

1104-
namespace internal {
1105-
1106-
void RegisterSourceNode(ExecFactoryRegistry*);
1107-
void RegisterFetchNode(ExecFactoryRegistry*);
1108-
void RegisterFilterNode(ExecFactoryRegistry*);
1109-
void RegisterOrderByNode(ExecFactoryRegistry*);
1110-
void RegisterPivotLongerNode(ExecFactoryRegistry*);
1111-
void RegisterProjectNode(ExecFactoryRegistry*);
1112-
void RegisterUnionNode(ExecFactoryRegistry*);
1113-
void RegisterAggregateNode(ExecFactoryRegistry*);
1114-
void RegisterSinkNode(ExecFactoryRegistry*);
1115-
void RegisterHashJoinNode(ExecFactoryRegistry*);
1116-
void RegisterAsofJoinNode(ExecFactoryRegistry*);
1117-
void RegisterSortedMergeNode(ExecFactoryRegistry*);
1118-
1119-
} // namespace internal
1120-
11211105
ExecFactoryRegistry* default_exec_factory_registry() {
11221106
class DefaultRegistry : public ExecFactoryRegistry {
11231107
public:

‎cpp/src/arrow/acero/exec_plan_internal.h‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
#pragma once
19+
20+
#include "arrow/acero/exec_plan.h"
21+
22+
namespace arrow::acero::internal {
23+
24+
void RegisterSourceNode(ExecFactoryRegistry*);
25+
void RegisterFetchNode(ExecFactoryRegistry*);
26+
void RegisterFilterNode(ExecFactoryRegistry*);
27+
void RegisterOrderByNode(ExecFactoryRegistry*);
28+
void RegisterPivotLongerNode(ExecFactoryRegistry*);
29+
void RegisterProjectNode(ExecFactoryRegistry*);
30+
void RegisterUnionNode(ExecFactoryRegistry*);
31+
void RegisterAggregateNode(ExecFactoryRegistry*);
32+
void RegisterSinkNode(ExecFactoryRegistry*);
33+
void RegisterHashJoinNode(ExecFactoryRegistry*);
34+
void RegisterAsofJoinNode(ExecFactoryRegistry*);
35+
void RegisterSortedMergeNode(ExecFactoryRegistry*);
36+
37+
} // namespace arrow::acero::internal

‎cpp/src/arrow/acero/fetch_node.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
#include "arrow/acero/accumulation_queue.h"
2121
#include "arrow/acero/exec_plan.h"
22+
#include "arrow/acero/exec_plan_internal.h"
2223
#include "arrow/acero/map_node.h"
2324
#include "arrow/acero/options.h"
2425
#include "arrow/acero/query_context.h"

‎cpp/src/arrow/acero/filter_node.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
// under the License.
1717

1818
#include "arrow/acero/exec_plan.h"
19+
#include "arrow/acero/exec_plan_internal.h"
1920
#include "arrow/acero/map_node.h"
2021
#include "arrow/acero/options.h"
2122
#include "arrow/acero/query_context.h"

‎cpp/src/arrow/acero/hash_join_node.cc‎

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <utility>
2222

2323
#include "arrow/acero/exec_plan.h"
24+
#include "arrow/acero/exec_plan_internal.h"
2425
#include "arrow/acero/hash_join.h"
2526
#include "arrow/acero/hash_join_dict.h"
2627
#include "arrow/acero/hash_join_node.h"
@@ -45,6 +46,24 @@ using compute::KeyColumnArray;
4546

4647
namespace acero {
4748

49+
namespace {
50+
51+
Status ValidateHashJoinNodeOptions(const HashJoinNodeOptions& join_options) {
52+
if (join_options.key_cmp.empty() || join_options.left_keys.empty() ||
53+
join_options.right_keys.empty()) {
54+
return Status::Invalid("key_cmp and keys cannot be empty");
55+
}
56+
57+
if ((join_options.key_cmp.size() != join_options.left_keys.size()) ||
58+
(join_options.key_cmp.size() != join_options.right_keys.size())) {
59+
return Status::Invalid("key_cmp and keys must have the same size");
60+
}
61+
62+
return Status::OK();
63+
}
64+
65+
} // namespace
66+
4867
// Check if a type is supported in a join (as either a key or non-key column)
4968
bool HashJoinSchema::IsTypeSupported(const DataType& type) {
5069
const Type::type id = type.id();
@@ -468,20 +487,6 @@ Status HashJoinSchema::CollectFilterColumns(std::vector<FieldRef>& left_filter,
468487
return Status::OK();
469488
}
470489

471-
Status ValidateHashJoinNodeOptions(const HashJoinNodeOptions& join_options) {
472-
if (join_options.key_cmp.empty() || join_options.left_keys.empty() ||
473-
join_options.right_keys.empty()) {
474-
return Status::Invalid("key_cmp and keys cannot be empty");
475-
}
476-
477-
if ((join_options.key_cmp.size() != join_options.left_keys.size()) ||
478-
(join_options.key_cmp.size() != join_options.right_keys.size())) {
479-
return Status::Invalid("key_cmp and keys must have the same size");
480-
}
481-
482-
return Status::OK();
483-
}
484-
485490
class HashJoinNode;
486491

487492
// This is a struct encapsulating things related to Bloom filters and pushing them around

‎cpp/src/arrow/acero/order_by_node.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include <vector>
2424

2525
#include "arrow/acero/exec_plan.h"
26+
#include "arrow/acero/exec_plan_internal.h"
2627
#include "arrow/acero/options.h"
2728
#include "arrow/acero/query_context.h"
2829
#include "arrow/acero/util.h"

0 commit comments

Comments
 (0)