This is an automated email from the ASF dual-hosted git repository.
apitrou pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/master by this push:
new ad1697e ARROW-4862: [C++] Fix gcc warnings in CHECKIN
ad1697e is described below
commit ad1697e5d25eeaff5630421f55b0120f45cf0ce1
Author: François Saint-Jacques <[email protected]>
AuthorDate: Thu Mar 21 14:32:10 2019 +0100
ARROW-4862: [C++] Fix gcc warnings in CHECKIN
- The recent change in DCHECK* (removing the ARROW_IGNORE_EXPR) added
new compilation warnings. The simplest way to disable this is to
disable unused variables or sprinkle ARROW_IGNORE_EXPR in a lot of
places. I went for the first.
- Added some checks to `system` return code in plasma tests
- Disable `-Wstrict-overflow` for ArrayBuilder::CheckCapacity
Author: François Saint-Jacques <[email protected]>
Closes #3976 from fsaintjacques/ARROW-4962-gcc-warnings and squashes the
following commits:
247e4a71e <François Saint-Jacques> Formating
64e87b484 <François Saint-Jacques> Abdicate to gcc
7bdf30cc1 <François Saint-Jacques> Add PLASMA_CHECK_SYSTEM
393920fce <François Saint-Jacques> Remove disable warning
b5853b519 <François Saint-Jacques> Improve NullBuilder warnings
fb775c7c4 <François Saint-Jacques> ARROW-4892: Fix gcc warnings in CHECKIN
---
cpp/cmake_modules/SetupCxxFlags.cmake | 2 +-
cpp/src/arrow/array/builder_base.h | 2 ++
cpp/src/arrow/array/builder_primitive.h | 12 +++---------
cpp/src/plasma/test-util.h | 7 +++++++
cpp/src/plasma/test/client_tests.cc | 12 +++++++-----
cpp/src/plasma/test/external_store_tests.cc | 12 +++++++-----
6 files changed, 27 insertions(+), 20 deletions(-)
diff --git a/cpp/cmake_modules/SetupCxxFlags.cmake
b/cpp/cmake_modules/SetupCxxFlags.cmake
index deb2742..e9d2695 100644
--- a/cpp/cmake_modules/SetupCxxFlags.cmake
+++ b/cpp/cmake_modules/SetupCxxFlags.cmake
@@ -164,7 +164,7 @@ if("${BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-unknown-warning-option
-Werror")
elseif("${COMPILER_FAMILY}" STREQUAL "gcc")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wall \
--Wno-conversion -Wno-sign-conversion")
+-Wno-conversion -Wno-sign-conversion -Wno-unused-variable")
# Treat all compiler warnings as errors
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Werror")
diff --git a/cpp/src/arrow/array/builder_base.h
b/cpp/src/arrow/array/builder_base.h
index ebe1ece..ee35407 100644
--- a/cpp/src/arrow/array/builder_base.h
+++ b/cpp/src/arrow/array/builder_base.h
@@ -170,9 +170,11 @@ class ARROW_EXPORT ArrayBuilder {
if (new_capacity < 0) {
return Status::Invalid("Resize capacity must be positive");
}
+
if (new_capacity < old_capacity) {
return Status::Invalid("Resize cannot downsize");
}
+
return Status::OK();
}
diff --git a/cpp/src/arrow/array/builder_primitive.h
b/cpp/src/arrow/array/builder_primitive.h
index 1025b4e..21f87b2 100644
--- a/cpp/src/arrow/array/builder_primitive.h
+++ b/cpp/src/arrow/array/builder_primitive.h
@@ -34,20 +34,14 @@ class ARROW_EXPORT NullBuilder : public ArrayBuilder {
/// \brief Append the specified number of null elements
Status AppendNulls(int64_t length) {
- auto new_length = length_ + length;
- ARROW_RETURN_NOT_OK(CheckCapacity(new_length, length_));
+ if (length < 0) return Status::Invalid("length must be positive");
null_count_ += length;
- length_ = new_length;
+ length_ += length;
return Status::OK();
}
/// \brief Append a single null element
- Status AppendNull() {
- ARROW_RETURN_NOT_OK(CheckCapacity(length_ + 1, length_));
- ++null_count_;
- ++length_;
- return Status::OK();
- }
+ Status AppendNull() { return AppendNulls(1); }
Status Append(std::nullptr_t) { return AppendNull(); }
diff --git a/cpp/src/plasma/test-util.h b/cpp/src/plasma/test-util.h
index 6bd501b..3173d94 100644
--- a/cpp/src/plasma/test-util.h
+++ b/cpp/src/plasma/test-util.h
@@ -37,6 +37,13 @@ ObjectID random_object_id() {
return result;
}
+#define PLASMA_CHECK_SYSTEM(expr) \
+ do { \
+ int status__ = (expr); \
+ EXPECT_TRUE(WIFEXITED(status__)); \
+ EXPECT_EQ(WEXITSTATUS(status__), 0); \
+ } while (false);
+
} // namespace plasma
#endif // PLASMA_TEST_UTIL_H
diff --git a/cpp/src/plasma/test/client_tests.cc
b/cpp/src/plasma/test/client_tests.cc
index d708e94..4dd0c06 100644
--- a/cpp/src/plasma/test/client_tests.cc
+++ b/cpp/src/plasma/test/client_tests.cc
@@ -62,7 +62,7 @@ class TestPlasmaStore : public ::testing::Test {
std::string plasma_command =
plasma_directory + "/plasma_store_server -m 10000000 -s " +
store_socket_name_ +
" 1> /dev/null 2> /dev/null & " + "echo $! > " + store_socket_name_ +
".pid";
- system(plasma_command.c_str());
+ PLASMA_CHECK_SYSTEM(system(plasma_command.c_str()));
ARROW_CHECK_OK(client_.Connect(store_socket_name_, ""));
ARROW_CHECK_OK(client2_.Connect(store_socket_name_, ""));
}
@@ -73,12 +73,14 @@ class TestPlasmaStore : public ::testing::Test {
#ifdef COVERAGE_BUILD
// Ask plasma_store to exit gracefully and give it time to write out
// coverage files
- std::string plasma_term_command = "kill -TERM `cat " + store_socket_name_
+ ".pid`";
- system(plasma_term_command.c_str());
+ std::string plasma_term_command =
+ "kill -TERM `cat " + store_socket_name_ + ".pid` || exit 0";
+ PLASMA_CHECK_SYSTEM(system(plasma_term_command.c_str()));
std::this_thread::sleep_for(std::chrono::milliseconds(200));
#endif
- std::string plasma_kill_command = "kill -KILL `cat " + store_socket_name_
+ ".pid`";
- system(plasma_kill_command.c_str());
+ std::string plasma_kill_command =
+ "kill -KILL `cat " + store_socket_name_ + ".pid` || exit 0";
+ PLASMA_CHECK_SYSTEM(system(plasma_kill_command.c_str()));
}
void CreateObject(PlasmaClient& client, const ObjectID& object_id,
diff --git a/cpp/src/plasma/test/external_store_tests.cc
b/cpp/src/plasma/test/external_store_tests.cc
index 5282328..26b10e2 100644
--- a/cpp/src/plasma/test/external_store_tests.cc
+++ b/cpp/src/plasma/test/external_store_tests.cc
@@ -64,7 +64,7 @@ class TestPlasmaStoreWithExternal : public ::testing::Test {
"hashtable://test -s " + store_socket_name_ +
" 1> /tmp/log.stdout 2> /tmp/log.stderr & " +
"echo $! > " + store_socket_name_ + ".pid";
- system(plasma_command.c_str());
+ PLASMA_CHECK_SYSTEM(system(plasma_command.c_str()));
ARROW_CHECK_OK(client_.Connect(store_socket_name_, ""));
}
void TearDown() override {
@@ -73,12 +73,14 @@ class TestPlasmaStoreWithExternal : public ::testing::Test {
#ifdef COVERAGE_BUILD
// Ask plasma_store to exit gracefully and give it time to write out
// coverage files
- std::string plasma_term_command = "kill -TERM `cat " + store_socket_name_
+ ".pid`";
- system(plasma_term_command.c_str());
+ std::string plasma_term_command =
+ "kill -TERM `cat " + store_socket_name_ + ".pid` || exit 0";
+ PLASMA_CHECK_SYSTEM(system(plasma_term_command.c_str()));
std::this_thread::sleep_for(std::chrono::milliseconds(200));
#endif
- std::string plasma_kill_command = "kill -KILL `cat " + store_socket_name_
+ ".pid`";
- system(plasma_kill_command.c_str());
+ std::string plasma_kill_command =
+ "kill -KILL `cat " + store_socket_name_ + ".pid` || exit 0";
+ PLASMA_CHECK_SYSTEM(system(plasma_kill_command.c_str()));
}
protected:
