Fix some warnings on GCC7

I tried compiling with GCC7 to see what warnings popped up.

Fix some ambiguous else warnings resulting from gtest macros. See
google/googletest#1119.

Add a missing include that broke compilation on the release build.

Fix some warnings that detect missing returns when there is a DCHECK
(these warnings already occurred in release builds, but they now
happen in gcc7 debug builds).

Change-Id: I39a12bc5ed6957c147b7f0dba85c7687cc989439
Reviewed-on: http://gerrit.cloudera.org:8080/12132
Reviewed-by: Impala Public Jenkins <[email protected]>
Tested-by: Impala Public Jenkins <[email protected]>

Author: timarmstrong

be/src/exec/delimited-text-parser-test.cc

@@ -42,7 +42,9 @@ void Validate(TupleDelimitedTextParser* parser, const string& data,
   } else {
     EXPECT_TRUE(data[offset - 1] == '\n' || data[offset - 1] == '\r')
         << data[offset - 1] << endl << data;
-    if (data[offset - 1] == '\r' && offset < data.size()) EXPECT_NE(data[offset], '\n');
+    if (data[offset - 1] == '\r' && offset < data.size()) {
+      EXPECT_NE(data[offset], '\n');
+    }
   }
 
   data_ptr += offset;

be/src/exec/hash-table-test.cc

@@ -170,7 +170,9 @@ class HashTableTest : public testing::Test {
       int32_t val = *reinterpret_cast<int32_t*>(build_expr_evals_[0]->GetValue(row));
       EXPECT_GE(val, min);
       EXPECT_LT(val, max);
-      if (all_unique) EXPECT_TRUE(results[val] == nullptr);
+      if (all_unique) {
+        EXPECT_TRUE(results[val] == nullptr);
+      }
       EXPECT_EQ(row->GetTuple(0), expected[val]->GetTuple(0));
       results[val] = row;
       iter.Next();

be/src/exprs/decimal-operators-ir.cc

@@ -582,7 +582,8 @@ IR_ALWAYS_INLINE DecimalVal DecimalOperators::CastToDecimalVal(
     return DecimalVal::null();
   }
 
-  DCHECK(result == StringParser::PARSE_SUCCESS || StringParser::PARSE_UNDERFLOW);
+  DCHECK(result == StringParser::PARSE_SUCCESS
+      || result == StringParser::PARSE_UNDERFLOW);
   return dv;
 }
 

be/src/exprs/string-functions-ir.cc

@@ -18,6 +18,7 @@
 #include "exprs/string-functions.h"
 
 #include <cctype>
+#include <numeric>
 #include <stdint.h>
 #include <re2/re2.h>
 #include <re2/stringpiece.h>

be/src/exprs/timezone_db.cc

@@ -24,6 +24,7 @@
 #include <regex>
 #include <boost/algorithm/string.hpp>
 
+#include "common/compiler-util.h"
 #include "common/logging.h"
 #include "kudu/util/path_util.h"
 #include "gutil/strings/ascii_ctype.h"
@@ -214,7 +215,7 @@ Status TimezoneDatabase::LoadZoneInfoFromHdfs(const string& hdfs_zone_info_zip,
   Status status = CopyHdfsFile(hdfs_conn, hdfs_zone_info_zip, local_conn,
       local_path.data());
   if (!status.ok()) {
-    (void)FileSystemUtil::RemovePaths({local_path.data()});
+    discard_result(FileSystemUtil::RemovePaths({local_path.data()}));
     return status;
   }
 

be/src/runtime/bufferpool/reservation-tracker-test.cc

@@ -412,7 +412,9 @@ TEST_F(ReservationTrackerTest, MemTrackerIntegrationMultiLevel) {
     ASSERT_TRUE(reservations[level].IncreaseReservation(amount));
     ASSERT_EQ(amount, reservations[level].GetReservation());
     ASSERT_EQ(0, reservations[level].GetUsedReservation());
-    if (level != 0) ASSERT_EQ(amount, mem_trackers[level]->consumption());
+    if (level != 0) {
+      ASSERT_EQ(amount, mem_trackers[level]->consumption());
+    }
     for (int ancestor = 0; ancestor < level; ++ancestor) {
       ASSERT_EQ(0, reservations[ancestor].GetUsedReservation());
       ASSERT_EQ(amount, reservations[ancestor].GetChildReservations());

be/src/runtime/fragment-instance-state.cc

@@ -469,7 +469,7 @@ void FragmentInstanceState::PublishFilter(const TPublishFilterParams& params) {
   runtime_state_->filter_bank()->PublishGlobalFilter(params);
 }
 
-string FragmentInstanceState::ExecStateToString(FInstanceExecStatePB state) {
+const string& FragmentInstanceState::ExecStateToString(FInstanceExecStatePB state) {
   // Labels to send to the debug webpages to display the current state to the user.
   static const string finstance_state_labels[] = {
       "Waiting for Exec",         // WAITING_FOR_EXEC
@@ -483,10 +483,10 @@ string FragmentInstanceState::ExecStateToString(FInstanceExecStatePB state) {
       "Finished"                  // FINISHED
   };
   /// Make sure we have a label for every possible state.
-  static_assert(sizeof(finstance_state_labels) / sizeof(char*) ==
+  static_assert(sizeof(finstance_state_labels) / sizeof(string) ==
       FInstanceExecStatePB::FINISHED + 1, "");
 
-  DCHECK_LT(state, sizeof(finstance_state_labels) / sizeof(char*))
+  DCHECK_LT(state, sizeof(finstance_state_labels) / sizeof(string))
       << "Unknown instance state";
   return finstance_state_labels[state];
 }

be/src/runtime/fragment-instance-state.h

@@ -101,7 +101,7 @@ class FragmentInstanceState {
   PlanRootSink* root_sink() { return root_sink_; }
 
   /// Returns a string description of 'state'.
-  static string ExecStateToString(FInstanceExecStatePB state);
+  static const string& ExecStateToString(FInstanceExecStatePB state);
 
   /// Name of the counter that is tracking per query, per host peak mem usage.
   /// TODO: this doesn't look like it belongs here

be/src/service/impala-server.cc

@@ -1452,8 +1452,9 @@ Status ImpalaServer::AuthorizeProxyUser(const string& user, const string& do_as_
   if (authorized_proxy_user_config_.size() == 0 &&
       authorized_proxy_group_config_.size() == 0) {
     error_msg << " User/group delegation is disabled.";
-    VLOG(1) << error_msg;
-    return Status::Expected(error_msg.str());
+    string error_msg_str = error_msg.str();
+    VLOG(1) << error_msg_str;
+    return Status::Expected(error_msg_str);
   }
 
   // Get the short version of the user name (the user name up to the first '/' or '@')
@@ -1505,8 +1506,9 @@ Status ImpalaServer::AuthorizeProxyUser(const string& user, const string& do_as_
     }
   }
 
-  VLOG(1) << error_msg;
-  return Status::Expected(error_msg.str());
+  string error_msg_str = error_msg.str();
+  VLOG(1) << error_msg_str;
+  return Status::Expected(error_msg_str);
 }
 
 void ImpalaServer::CatalogUpdateVersionInfo::UpdateCatalogVersionMetrics()

be/src/statestore/statestore.cc

@@ -832,6 +832,7 @@ const char* Statestore::GetUpdateKindName(UpdateKind kind) {
       return "heartbeat";
   }
   DCHECK(false);
+  return "";
 }
 
 ThreadPool<Statestore::ScheduledSubscriberUpdate>* Statestore::GetThreadPool(
@@ -845,6 +846,7 @@ ThreadPool<Statestore::ScheduledSubscriberUpdate>* Statestore::GetThreadPool(
       return &subscriber_heartbeat_threadpool_;
   }
   DCHECK(false);
+  return nullptr;
 }
 
 Status Statestore::SendHeartbeat(Subscriber* subscriber) {

be/src/util/decompress-test.cc

@@ -229,7 +229,9 @@ class DecompressorTest : public ::testing::Test {
 
     EXPECT_EQ(0, compressed_bytes_remaining);
     EXPECT_EQ(stream_end, expected_stream_end);
-    if (stream_end) EXPECT_EQ(decompressed_len, uncompressed_len);
+    if (stream_end) {
+      EXPECT_EQ(decompressed_len, uncompressed_len);
+    }
     if (bytes_decompressed != NULL) *bytes_decompressed = decompressed_len;
 
     return Status::OK();

be/src/util/metrics-test.cc

@@ -267,8 +267,12 @@ void AssertJson(const Value& val, const string& name, const string& value,
   EXPECT_EQ(val["name"].GetString(), name);
   EXPECT_EQ(val["human_readable"].GetString(), value);
   EXPECT_EQ(val["description"].GetString(), description);
-  if (!kind.empty()) EXPECT_EQ(val["kind"].GetString(), kind);
-  if (!units.empty()) EXPECT_EQ(val["units"].GetString(), units);
+  if (!kind.empty()) {
+    EXPECT_EQ(val["kind"].GetString(), kind);
+  }
+  if (!units.empty()) {
+    EXPECT_EQ(val["units"].GetString(), units);
+  }
 }
 
 TEST_F(MetricsTest, CountersJson) {

be/src/util/webserver.cc

@@ -505,7 +505,9 @@ const string Webserver::GetMimeType(const ContentType& content_type) {
     case HTML: return "text/html; charset=UTF-8";
     case PLAIN: return "text/plain; charset=UTF-8";
     case JSON: return "application/json";
-    default: DCHECK(false) << "Invalid content_type: " << content_type;
+    default:
+      DCHECK(false) << "Invalid content_type: " << content_type;
+      return "";
   }
 }
 }