Skip to content
Permalink
Browse files

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 <fsaintjacques@gmail.com>

Closes #3976 from fsaintjacques/ARROW-4962-gcc-warnings and squashes the following commits:

247e4a7 <François Saint-Jacques> Formating
64e87b4 <François Saint-Jacques> Abdicate to gcc
7bdf30c <François Saint-Jacques> Add PLASMA_CHECK_SYSTEM
393920f <François Saint-Jacques> Remove disable warning
b5853b5 <François Saint-Jacques> Improve NullBuilder warnings
fb775c7 <François Saint-Jacques> ARROW-4892:  Fix gcc warnings in CHECKIN
  • Loading branch information
fsaintjacques authored and pitrou committed Mar 21, 2019
1 parent 376c9ad commit ad1697e5d25eeaff5630421f55b0120f45cf0ce1
@@ -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")
@@ -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();
}

@@ -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(); }

@@ -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
@@ -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,
@@ -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:

0 comments on commit ad1697e

Please sign in to comment.
You can’t perform that action at this time.