[yugabyte#9279] YSQL: Enable -Wextra on pgwrapper

Summary:
As a part of enabling -Wextra and some other flags from RocksDB on entire code base this diff
enables flags on pgwrapper directory.

All new warnings are fixed in context of this diff. Compilation was tested on GCC9.

Test Plan: Jenkins

Reviewers: sergei, alex

Reviewed By: alex

Subscribers: yql, bogdan

Differential Revision: https://phabricator.dev.yugabyte.com/D12747

src/yb/consensus/consensus_meta.cc

@@ -113,7 +113,7 @@ Status ConsensusMetadata::DeleteOnDiskData(FsManager* fs_manager, const string&
   return Status::OK();
 }
 
-const int64_t ConsensusMetadata::current_term() const {
+int64_t ConsensusMetadata::current_term() const {
   DCHECK(pb_.has_current_term());
   return pb_.current_term();
 }

src/yb/consensus/consensus_meta.h

@@ -95,7 +95,7 @@ class ConsensusMetadata {
   static CHECKED_STATUS DeleteOnDiskData(FsManager* fs_manager, const std::string& tablet_id);
 
   // Accessors for current term.
-  const int64_t current_term() const;
+  int64_t current_term() const;
   void set_current_term(int64_t term);
 
   // Accessors for voted_for.

src/yb/consensus/log.cc

@@ -1251,7 +1251,7 @@ Status Log::Close() {
   }
 }
 
-const int Log::num_segments() const {
+int Log::num_segments() const {
   boost::shared_lock<rw_spinlock> read_lock(state_lock_.get_lock());
   return (reader_) ? reader_->num_segments() : 0;
 }

src/yb/consensus/log.h

@@ -277,7 +277,7 @@ class Log : public RefCountedThreadSafe<Log> {
   CHECKED_STATUS TEST_SubmitFuncToAppendToken(const std::function<void()>& func);
 
   // Returns the number of segments.
-  const int num_segments() const;
+  int num_segments() const;
 
   const std::string& LogPrefix() const {
     return log_prefix_;

src/yb/consensus/log_util.cc

@@ -225,7 +225,7 @@ Status ReadableLogSegment::Init() {
   return Status::OK();
 }
 
-const int64_t ReadableLogSegment::readable_up_to() const {
+int64_t ReadableLogSegment::readable_up_to() const {
   return readable_to_offset_.Load();
 }
 

src/yb/consensus/log_util.h

@@ -246,30 +246,30 @@ class ReadableLogSegment : public RefCountedThreadSafe<ReadableLogSegment> {
     return footer_;
   }
 
-  const std::shared_ptr<RandomAccessFile> readable_file() const {
+  std::shared_ptr<RandomAccessFile> readable_file() const {
     return readable_file_;
   }
 
-  const std::shared_ptr<RandomAccessFile> readable_file_checkpoint() const {
+  std::shared_ptr<RandomAccessFile> readable_file_checkpoint() const {
     return readable_file_checkpoint_;
   }
 
-  const int64_t file_size() const {
+  int64_t file_size() const {
     return file_size_.Load();
   }
 
-  const int64_t first_entry_offset() const {
+  int64_t first_entry_offset() const {
     return first_entry_offset_;
   }
 
-  const int64_t get_header_size() const {
+  int64_t get_header_size() const {
     return readable_file_->GetEncryptionHeaderSize();
   }
 
   // Returns the full size of the file, if the segment is closed and has
   // a footer, or the offset where the last written, non corrupt entry
   // ends.
-  const int64_t readable_up_to() const;
+  int64_t readable_up_to() const;
 
  private:
   friend class RefCountedThreadSafe<ReadableLogSegment>;
@@ -433,11 +433,11 @@ class WritableLogSegment {
     return path_;
   }
 
-  const int64_t first_entry_offset() const {
+  int64_t first_entry_offset() const {
     return first_entry_offset_;
   }
 
-  const int64_t written_offset() const {
+  int64_t written_offset() const {
     return written_offset_;
   }
 

src/yb/docdb/value.h

@@ -67,7 +67,7 @@ class Value {
 
   const PrimitiveValue& primitive_value() const { return primitive_value_; }
 
-  const uint64_t merge_flags() const { return merge_flags_; }
+  uint64_t merge_flags() const { return merge_flags_; }
 
   const DocHybridTime& intent_doc_ht() const { return intent_doc_ht_; }
 

src/yb/integration-tests/mini_cluster.h

@@ -92,12 +92,12 @@ struct MiniClusterOptions {
   rocksdb::Env* ts_rocksdb_env = nullptr;
 
   // Directory in which to store data.
-  // Default: "", which auto-generates a unique path for this cluster.
+  // Default: empty string, which auto-generates a unique path for this cluster.
   // The default may only be used from a gtest unit test.
-  std::string data_root;
+  std::string data_root{};
 
   // Cluster id used to create fs path when we create tests with multiple clusters.
-  std::string cluster_id = "";
+  std::string cluster_id{};
 };
 
 // An in-process cluster with a MiniMaster and a configurable

src/yb/master/catalog_entity_info.h

@@ -349,7 +349,7 @@ struct PersistentTableInfo : public Persistent<SysTablesEntryPB, SysRowEntry::TA
   }
 
   // Return the table's type.
-  const TableType table_type() const {
+  TableType table_type() const {
     return pb.table_type();
   }
 

src/yb/tablet/tablet_peer.cc

@@ -407,7 +407,7 @@ Status TabletPeer::Start(const ConsensusBootstrapInfo& bootstrap_info) {
   return tablet_->EnableCompactions(/* non_abortable_ops_pause */ nullptr);
 }
 
-const consensus::RaftConfigPB TabletPeer::RaftConfig() const {
+consensus::RaftConfigPB TabletPeer::RaftConfig() const {
   CHECK(consensus_) << "consensus is null";
   return consensus_->CommittedConfig();
 }
@@ -719,7 +719,7 @@ Status TabletPeer::RunLogGC() {
   return log_->GC(min_log_index, &num_gced);
 }
 
-const TabletDataState TabletPeer::data_state() const {
+TabletDataState TabletPeer::data_state() const {
   std::lock_guard<simple_spinlock> lock(lock_);
   return meta_->tablet_data_state();
 }

src/yb/tablet/tablet_peer.h

@@ -244,14 +244,14 @@ class TabletPeer : public consensus::ConsensusContext,
     return tablet_;
   }
 
-  const RaftGroupStatePB state() const {
+  RaftGroupStatePB state() const {
     return state_.load(std::memory_order_acquire);
   }
 
-  const TabletDataState data_state() const;
+  TabletDataState data_state() const;
 
   // Returns the current Raft configuration.
-  const consensus::RaftConfigPB RaftConfig() const;
+  consensus::RaftConfigPB RaftConfig() const;
 
   TabletStatusListener* status_listener() const {
     return status_listener_.get();

src/yb/util/auto_release_pool.h

@@ -85,7 +85,7 @@ class AutoReleasePool {
 
   template <class T>
   struct SpecificElement : GenericElement {
-    explicit SpecificElement(T *t): t(t) {}
+    explicit SpecificElement(T *t_): t(t_) {}
     ~SpecificElement() {
       delete t;
     }
@@ -95,7 +95,7 @@ class AutoReleasePool {
 
   template <class T>
   struct SpecificArrayElement : GenericElement {
-    explicit SpecificArrayElement(T *t): t(t) {}
+    explicit SpecificArrayElement(T *t_): t(t_) {}
     ~SpecificArrayElement() {
       delete [] t;
     }
@@ -110,4 +110,5 @@ class AutoReleasePool {
 
 
 } // namespace yb
-#endif
+
+#endif // YB_UTIL_AUTO_RELEASE_POOL_H

src/yb/util/test_macros.h

@@ -96,22 +96,22 @@ std::string TEST_SetDifferenceStr(const std::set<T>& expected, const std::set<T>
 // RocksDB's Status types.
 
 #define ASSERT_OK(status) do { \
-    auto&& _s = (status); \
-    if (_s.ok()) { \
+    auto&& _assert_status = (status); \
+    if (_assert_status.ok()) { \
       SUCCEED(); \
     } else { \
-      FAIL() << "Bad status: " << StatusToString(_s);  \
+      FAIL() << "Bad status: " << StatusToString(_assert_status);  \
     } \
   } while (0)
 
 #define ASSERT_NOK(s) ASSERT_FALSE((s).ok())
 
 #define ASSERT_OK_PREPEND(status, msg) do { \
-  auto&& _s = (status); \
-  if (_s.ok()) { \
+  auto&& _assert_status = (status); \
+  if (_assert_status.ok()) { \
     SUCCEED(); \
   } else { \
-    FAIL() << (msg) << " - status: " << StatusToString(_s);  \
+    FAIL() << (msg) << " - status: " << StatusToString(_assert_status);  \
   } \
 } while (0)
 
@@ -133,9 +133,9 @@ std::string TEST_SetDifferenceStr(const std::set<T>& expected, const std::set<T>
 // Like the above, but doesn't record successful
 // tests.
 #define ASSERT_OK_FAST(status) do {      \
-    auto&& _s = (status); \
-    if (!_s.ok()) { \
-      FAIL() << "Bad status: " << StatusToString(_s);  \
+    auto&& _assert_status = (status); \
+    if (!_assert_status.ok()) { \
+      FAIL() << "Bad status: " << StatusToString(_assert_status);  \
     } \
   } while (0)
 

src/yb/yql/pgwrapper/CMakeLists.txt

@@ -11,6 +11,9 @@
 # under the License.
 #
 
+# TODO(dmitry): Remove next line after fixing #9279
+set(CMAKE_CXX_FLAGS "${YB_CMAKE_CXX_EXTRA_FLAGS} ${CMAKE_CXX_FLAGS}")
+
 set(PGWRAPPER_SRCS
     pg_wrapper.cc)
 set(PGWRAPPER_LIBS

src/yb/yql/pgwrapper/libpq_utils.cc

@@ -221,7 +221,7 @@ Result<PGResultPtr> CheckResult(PGResultPtr result, const std::string& command)
                   Slice() /* msg2 */,
                   PgsqlError(GetSqlState(result.get())));
   }
-  return std::move(result);
+  return result;
 }
 
 Result<PGResultPtr> PGConn::Fetch(const std::string& command) {

src/yb/yql/pgwrapper/pg_index_backfill-test.cc

@@ -138,13 +138,13 @@ void PgIndexBackfillTest::TestSimpleBackfill(const std::string& table_create_suf
 void PgIndexBackfillTest::TestRetainDeleteMarkers(const std::string& db_name) {
   auto client = ASSERT_RESULT(cluster_->CreateClient());
 
-  const std::string& kIndexName = "ttt_idx";
-  ASSERT_OK(conn_->ExecuteFormat("CREATE TABLE $0 (i int)", kTableName));
-  ASSERT_OK(conn_->ExecuteFormat("CREATE INDEX $0 ON $1 (i ASC)", kIndexName, kTableName));
+  const std::string index_name = "ttt_idx";
+  ASSERT_OK(conn_->ExecuteFormat("CREATE TABLE $0 (i int)", index_name));
+  ASSERT_OK(conn_->ExecuteFormat("CREATE INDEX $0 ON $1 (i ASC)", index_name, kTableName));
 
   // Verify that retain_delete_markers was set properly in the index table schema.
   const std::string table_id = ASSERT_RESULT(GetTableIdByTableName(
-      client.get(), db_name, kIndexName));
+      client.get(), db_name, index_name));
   auto table_info = std::make_shared<client::YBTableInfo>();
   {
     Synchronizer sync;

src/yb/yql/pgwrapper/pg_libpq-test.cc

@@ -249,9 +249,9 @@ TEST_F(PgLibPqTest, YB_DISABLE_TEST_IN_TSAN(SerializableColoring)) {
     SCOPED_TRACE(iteration_title);
     LOG(INFO) << iteration_title;
 
-    auto status = conn.Execute("DELETE FROM t");
-    if (!status.ok()) {
-      ASSERT_STR_CONTAINS(status.ToString(), kTryAgain);
+    auto s = conn.Execute("DELETE FROM t");
+    if (!s.ok()) {
+      ASSERT_STR_CONTAINS(s.ToString(), kTryAgain);
       continue;
     }
     for (int k = 0; k != kKeys; ++k) {
@@ -264,12 +264,12 @@ TEST_F(PgLibPqTest, YB_DISABLE_TEST_IN_TSAN(SerializableColoring)) {
     for (int i = 0; i != kColors; ++i) {
       int32_t color = i;
       threads.emplace_back([this, color, kKeys, &complete] {
-        auto conn = ASSERT_RESULT(Connect());
+        auto connection = ASSERT_RESULT(Connect());
 
-        ASSERT_OK(conn.Execute("BEGIN"));
-        ASSERT_OK(conn.Execute("SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"));
+        ASSERT_OK(connection.Execute("BEGIN"));
+        ASSERT_OK(connection.Execute("SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"));
 
-        auto res = conn.Fetch("SELECT * FROM t");
+        auto res = connection.Fetch("SELECT * FROM t");
         if (!res.ok()) {
           auto msg = res.status().message().ToBuffer();
           ASSERT_STR_CONTAINS(res.status().ToString(), kTryAgain);
@@ -280,13 +280,14 @@ TEST_F(PgLibPqTest, YB_DISABLE_TEST_IN_TSAN(SerializableColoring)) {
 
         auto lines = PQntuples(res->get());
         ASSERT_EQ(kKeys, lines);
-        for (int i = 0; i != lines; ++i) {
-          if (ASSERT_RESULT(GetInt32(res->get(), i, 1)) == color) {
+        for (int j = 0; j != lines; ++j) {
+          if (ASSERT_RESULT(GetInt32(res->get(), j, 1)) == color) {
             continue;
           }
 
-          auto key = ASSERT_RESULT(GetInt32(res->get(), i, 0));
-          auto status = conn.ExecuteFormat("UPDATE t SET color = $1 WHERE key = $0", key, color);
+          auto key = ASSERT_RESULT(GetInt32(res->get(), j, 0));
+          auto status = connection.ExecuteFormat(
+              "UPDATE t SET color = $1 WHERE key = $0", key, color);
           if (!status.ok()) {
             auto msg = status.message().ToBuffer();
             // Missing metadata means that transaction was aborted and cleaned.
@@ -296,7 +297,7 @@ TEST_F(PgLibPqTest, YB_DISABLE_TEST_IN_TSAN(SerializableColoring)) {
           }
         }
 
-        auto status = conn.Execute("COMMIT");
+        auto status = connection.Execute("COMMIT");
         if (!status.ok()) {
           auto msg = status.message().ToBuffer();
           ASSERT_TRUE(msg.find("Operation expired") != std::string::npos) << status;
@@ -648,15 +649,16 @@ void PgLibPqTest::TestMultiBankAccount(IsolationLevel isolation) {
   thread_holder.AddThreadFunctor(
       [this, &counter, &reads, &writes, isolation, &stop_flag = thread_holder.stop_flag()]() {
     SetFlagOnExit set_flag_on_exit(&stop_flag);
-    auto conn = ASSERT_RESULT(Connect());
+    auto connection = ASSERT_RESULT(Connect());
     auto failures_in_row = 0;
     while (!stop_flag.load(std::memory_order_acquire)) {
       if (isolation == IsolationLevel::SERIALIZABLE_ISOLATION) {
         auto lower_bound = reads.load() * kRequiredWrites < writes.load() * kRequiredReads
             ? 1.0 - 1.0 / (1ULL << failures_in_row) : 0.0;
-        ASSERT_OK(conn.ExecuteFormat("SET yb_transaction_priority_lower_bound = $0", lower_bound));
+        ASSERT_OK(connection.ExecuteFormat(
+            "SET yb_transaction_priority_lower_bound = $0", lower_bound));
       }
-      auto sum = ReadSumBalance(&conn, kAccounts, isolation, &counter);
+      auto sum = ReadSumBalance(&connection, kAccounts, isolation, &counter);
       if (!sum.ok()) {
         // Do not overflow long when doing bitshift above.
         failures_in_row = std::min(failures_in_row + 1, 63);
@@ -843,14 +845,14 @@ TEST_F(PgLibPqTest, YB_DISABLE_TEST_IN_TSAN(SecondaryIndexInsertSelect)) {
 
   for (int i = 0; i != kThreads; ++i) {
     holder.AddThread([this, i, &stop = holder.stop_flag(), &written] {
-      auto conn = ASSERT_RESULT(Connect());
+      auto connection = ASSERT_RESULT(Connect());
       int key = 0;
 
       while (!stop.load(std::memory_order_acquire)) {
         if (RandomUniformBool()) {
           int a = i * 1000000 + key;
           int b = key;
-          ASSERT_OK(conn.ExecuteFormat("INSERT INTO t (a, b) VALUES ($0, $1)", a, b));
+          ASSERT_OK(connection.ExecuteFormat("INSERT INTO t (a, b) VALUES ($0, $1)", a, b));
           written[i].store(++key, std::memory_order_release);
         } else {
           int writer_index = RandomUniformInt(0, kThreads - 1);
@@ -860,7 +862,7 @@ TEST_F(PgLibPqTest, YB_DISABLE_TEST_IN_TSAN(SecondaryIndexInsertSelect)) {
           }
           int read_key = num_written - 1;
           int b = read_key;
-          int read_a = ASSERT_RESULT(conn.FetchValue<int32_t>(
+          int read_a = ASSERT_RESULT(connection.FetchValue<int32_t>(
               Format("SELECT a FROM t WHERE b = $0 LIMIT 1", b)));
           ASSERT_EQ(read_a % 1000000, read_key);
         }
@@ -2150,8 +2152,8 @@ TEST_F_EX(PgLibPqTest,
   // inherit the new --TEST_do_not_add_enum_sort_order flag from the tablet
   // server.
   for (int i = 0; i < cluster_->num_tablet_servers(); ++i) {
-    ExternalTabletServer* pg_ts = cluster_->tablet_server(i);
-    const string pg_pid_file = JoinPathSegments(pg_ts->GetDataDir(), "pg_data",
+    ExternalTabletServer* ts = cluster_->tablet_server(i);
+    const string pg_pid_file = JoinPathSegments(ts->GetDataDir(), "pg_data",
                                                 "postmaster.pid");
 
     LOG(INFO) << "pg_pid_file: " << pg_pid_file;