Clang-tidy clean-ups (sub-libs of core vrs code)

Summary: clang-tidy makes many suggestions. - missing initializations - syntax changes - using modern C++ constructs - static methods when possible - using explict for constructors - "= default" constructors and destructors and other minor things. Differential Revision: D52745971 fbshipit-source-id: e88c924ff26e6bd3f94a3e0fc2f3d35245b490c6
facebookresearch · Jan 13, 2024 · 085003c · 085003c
1 parent 0959e24
commit 085003c
Show file tree

Hide file tree

Showing 47 changed files with 166 additions and 175 deletions.
diff --git a/vrs/helpers/FileMacros.h b/vrs/helpers/FileMacros.h
@@ -19,13 +19,14 @@
 // Macro to write a known number of bytes to a file, and return the error if necessary
 #define WRITE_OR_LOG_AND_RETURN(file_, data_, byteCount_)        \
   do {                                                           \
-    if (byteCount_ > 0) {                                        \
-      int error_ = file_.write(data_, byteCount_);               \
+    size_t __length = byteCount_;                                \
+    if (__length > 0) {                                          \
+      int error_ = (file_).write(data_, __length);               \
       if (error_ != 0) {                                         \
         XR_LOGE(                                                 \
             "File write error, {} instead of {}, Error: {}, {}", \
-            file_.getLastRWSize(),                               \
-            byteCount_,                                          \
+            (file_).getLastRWSize(),                             \
+            __length,                                            \
             error_,                                              \
             errorCodeToMessage(error_));                         \
         return error_;                                           \
@@ -36,7 +37,7 @@
 // Helper macro for some file operations
 #define IF_ERROR_LOG_AND_RETURN(operation_)                                                        \
   do {                                                                                             \
-    int operationError_ = operation_;                                                              \
+    int operationError_ = (operation_);                                                            \
     if (operationError_ != 0) {                                                                    \
       XR_LOGE(                                                                                     \
           "{} failed: {}, {}", #operation_, operationError_, errorCodeToMessage(operationError_)); \

diff --git a/vrs/helpers/JobQueue.h b/vrs/helpers/JobQueue.h
@@ -16,13 +16,12 @@
 
 #pragma once
 
-#include <algorithm>
 #include <atomic>
 #include <condition_variable>
+#include <deque>
 #include <functional>
 #include <memory>
 #include <mutex>
-#include <queue>
 #include <thread>
 
 #include <vrs/os/Time.h>
@@ -53,7 +52,7 @@ class JobQueue {
     }
     double limit = os::getTimestampSec() + waitTime;
     std::unique_lock<std::mutex> locker(mutex_);
-    double actualWaitTime;
+    double actualWaitTime = 0;
     while (!hasEnded_ && queue_.empty() && (actualWaitTime = limit - os::getTimestampSec()) >= 0) {
       condition_.wait_for(locker, std::chrono::duration<double>(actualWaitTime));
     }

diff --git a/vrs/helpers/MemBuffer.h b/vrs/helpers/MemBuffer.h
@@ -39,7 +39,7 @@ class MemBuffer {
  public:
   /// Create a MemBuffer with a minimum block allocation size.
   /// If that size is equal or greater than the total data, memory copies are minimized.
-  MemBuffer(size_t allocSize = 256 * 1024);
+  explicit MemBuffer(size_t allocSize = 256 * 1024);
 
   /// Add block of bytes
   /// @param data: pointer to the data

diff --git a/vrs/helpers/Rapidjson.hpp b/vrs/helpers/Rapidjson.hpp
@@ -148,7 +148,7 @@ template <typename JSTR>
 inline void
 serializeStringRefMap(const map<string, string>& stringMap, JsonWrapper& rj, const JSTR& name) {
   using namespace vrs_rapidjson;
-  if (stringMap.size() > 0) {
+  if (!stringMap.empty()) {
     JValue mapValues(kObjectType);
     for (const auto& element : stringMap) {
       mapValues.AddMember(jStringRef(element.first), jStringRef(element.second), rj.alloc);
@@ -160,7 +160,7 @@ serializeStringRefMap(const map<string, string>& stringMap, JsonWrapper& rj, con
 template <typename T, typename JSTR>
 inline void serializeVector(const vector<T>& vect, JsonWrapper& rj, const JSTR& name) {
   using namespace vrs_rapidjson;
-  if (vect.size() > 0) {
+  if (!vect.empty()) {
     JValue arrayValues(kArrayType);
     arrayValues.Reserve(static_cast<SizeType>(vect.size()), rj.alloc);
     for (const auto& element : vect) {
@@ -175,7 +175,7 @@ template <typename JSTR>
 inline void
 serializeStringRefVector(const vector<string>& vect, JsonWrapper& rj, const JSTR& name) {
   using namespace vrs_rapidjson;
-  if (vect.size() > 0) {
+  if (!vect.empty()) {
     JValue arrayValues(kArrayType);
     arrayValues.Reserve(static_cast<SizeType>(vect.size()), rj.alloc);
     for (const auto& str : vect) {

diff --git a/vrs/helpers/Serialization.h b/vrs/helpers/Serialization.h
@@ -20,7 +20,7 @@
 
 #define RAPIDJSON_NAMESPACE vrs_rapidjson
 #define RAPIDJSON_HAS_STDSTRING 1
-#define RAPIDJSON_PARSE_DEFAULT_FLAGS kParseFullPrecisionFlag | kParseNanAndInfFlag
+#define RAPIDJSON_PARSE_DEFAULT_FLAGS (kParseFullPrecisionFlag | kParseNanAndInfFlag)
 #define RAPIDJSON_WRITE_DEFAULT_FLAGS kWriteNanAndInfFlag
 
 #include <rapidjson/rapidjson.h>

diff --git a/vrs/helpers/test/MemBufferTest.cpp b/vrs/helpers/test/MemBufferTest.cpp
@@ -28,16 +28,16 @@ using namespace vrs::helpers;
 
 #define ADD(MB, PTR, SIZE) \
   MB.addData(PTR, SIZE);   \
-  PTR += SIZE
-
-#define ALLOCATE_ADD(MB, PTR, SIZE_ALLOC, SIZE_COPY)           \
-  {                                                            \
-    uint8_t* ptr_;                                             \
-    size_t allocatedSize = MB.allocateSpace(ptr_, SIZE_ALLOC); \
-    EXPECT_GE(allocatedSize, SIZE_ALLOC);                      \
-    memcpy(ptr_, PTR, SIZE_COPY);                              \
-    MB.addAllocatedSpace(SIZE_COPY);                           \
-    PTR += SIZE_COPY;                                          \
+  (PTR) += (SIZE)
+
+#define ALLOCATE_ADD(MB, PTR, SIZE_ALLOC, SIZE_COPY)             \
+  {                                                              \
+    uint8_t* ptr_;                                               \
+    size_t allocatedSize = (MB).allocateSpace(ptr_, SIZE_ALLOC); \
+    EXPECT_GE(allocatedSize, SIZE_ALLOC);                        \
+    memcpy(ptr_, PTR, SIZE_COPY);                                \
+    (MB).addAllocatedSpace(SIZE_COPY);                           \
+    (PTR) += (SIZE_COPY);                                        \
   }
 
 namespace {

diff --git a/vrs/helpers/test/StringsTest.cpp b/vrs/helpers/test/StringsTest.cpp
@@ -177,7 +177,7 @@ TEST_F(StringsHelpersTester, getValueTest) {
       {"double", "-3.5"},
       {"double_bad", "abc"},
   };
-  bool boolValue;
+  bool boolValue = false;
   EXPECT_TRUE(getBool(m, "bool_true", boolValue));
   EXPECT_EQ(boolValue, true);
   EXPECT_TRUE(getBool(m, "bool_false", boolValue));
@@ -186,26 +186,26 @@ TEST_F(StringsHelpersTester, getValueTest) {
   EXPECT_EQ(boolValue, false);
   EXPECT_FALSE(getBool(m, "nobool", boolValue));
 
-  int intValue;
+  int intValue = 0;
   EXPECT_TRUE(getInt(m, "int", intValue));
   EXPECT_EQ(intValue, 1234567890);
   EXPECT_FALSE(getInt(m, "noint", intValue));
 
-  int64_t int64Value;
+  int64_t int64Value = 0;
   EXPECT_TRUE(getInt64(m, "int64_pos", int64Value));
   EXPECT_EQ(int64Value, 1234567890);
   EXPECT_TRUE(getInt64(m, "int64_neg", int64Value));
   EXPECT_EQ(int64Value, -1234567890);
   EXPECT_FALSE(getInt64(m, "noint64", int64Value));
 
-  uint64_t uint64Value;
+  uint64_t uint64Value = 0;
   EXPECT_TRUE(getUInt64(m, "uint64", uint64Value));
   EXPECT_EQ(uint64Value, 1234567890);
   EXPECT_TRUE(getUInt64(m, "uint64_neg", uint64Value));
   EXPECT_EQ(uint64Value, 0xffffffffffffffff);
   EXPECT_FALSE(getUInt64(m, "nouint64", uint64Value));
 
-  double doubleValue;
+  double doubleValue = 0;
   EXPECT_TRUE(getDouble(m, "double", doubleValue));
   EXPECT_EQ(doubleValue, -3.5);
   EXPECT_FALSE(getDouble(m, "double_bad", doubleValue));
@@ -252,7 +252,7 @@ TEST_F(StringsHelpersTester, humanReadableTimestampTest) {
 
 TEST_F(StringsHelpersTester, readUnsignedInt32) {
   using namespace vrs::helpers;
-  uint32_t outInt;
+  uint32_t outInt = 0;
   const char* strInt = "123";
   readUInt32(strInt, outInt);
   EXPECT_EQ(outInt, 123);

diff --git a/vrs/os/Event.h b/vrs/os/Event.h
@@ -37,12 +37,12 @@ class EventChannel {
   static const uint32_t kInfiniteTimeout = std::numeric_limits<uint32_t>::max();
 
   /// Event represents an instance of an event.
-  typedef struct event_ {
+  struct Event {
     void* pointer;
     int64_t value;
     int32_t numMissedEvents;
     double timestampSec;
-  } Event;
+  };
 
   /// Construct EventChannel.
   /// @param notificationMode: whether event can be unicasted or broadcasted

diff --git a/vrs/os/Semaphore.cpp b/vrs/os/Semaphore.cpp
@@ -21,7 +21,7 @@
 namespace vrs {
 namespace os {
 
-bool Semaphore::timedWait(const double timeSec) {
+bool Semaphore::timedWait(double timeSec) {
   using namespace boost::posix_time;
   long integralTime = static_cast<long>(std::floor(timeSec));
   long microSeconds = static_cast<long>((timeSec - integralTime) * 1000000);

diff --git a/vrs/os/Semaphore.h b/vrs/os/Semaphore.h
@@ -31,7 +31,7 @@ class Semaphore : private boost::interprocess::interprocess_semaphore {
   using boost::interprocess::interprocess_semaphore::post;
   using boost::interprocess::interprocess_semaphore::wait;
 
-  bool timedWait(const double timeSec);
+  bool timedWait(double timeSec);
   bool timed_wait(const boost::posix_time::ptime& absTime) = delete;
 };
 

diff --git a/vrs/os/System.cpp b/vrs/os/System.cpp
@@ -58,7 +58,7 @@ string vrs::os::getOsFingerPrint() {
   return osFingerprintString;
 
 #elif IS_MAC_PLATFORM()
-  array<char, 256> osFingerprint;
+  array<char, 256> osFingerprint{};
   size_t size = osFingerprint.size();
   string osFingerprintString = "MacOS ";
   if (sysctlbyname("kern.osrelease", osFingerprint.data(), &size, nullptr, 0) == 0) {

diff --git a/vrs/os/Time.cpp b/vrs/os/Time.cpp
@@ -48,7 +48,7 @@ int64_t getTimestampMs() {
 
 bool getProcessCpuTimes(double& outUserCpuTime, double& outSystemCpuTime) {
 #if IS_MAC_PLATFORM() || IS_LINUX_PLATFORM()
-  struct rusage r_usage;
+  struct rusage r_usage {};
   getrusage(RUSAGE_SELF, &r_usage);
   const double cMicroseconds = 1e-6;
   outUserCpuTime = r_usage.ru_utime.tv_sec + r_usage.ru_utime.tv_usec * cMicroseconds;

diff --git a/vrs/os/Utils.cpp b/vrs/os/Utils.cpp
@@ -50,11 +50,12 @@
 #if IS_LINUX_PLATFORM()
 #include <linux/limits.h>
 #endif
-#include <stdlib.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
+
 #include <cerrno>
+#include <cstdlib>
 #include <cstring>
 #endif // !IS_WINDOWS_PLATFORM()
 
@@ -67,7 +68,6 @@ constexpr auto kRegularFileType = boost::filesystem::file_type::regular_file;
 #endif
 
 using std::string;
-using std::vector;
 
 namespace vrs {
 namespace os {

diff --git a/vrs/os/test/EventUnitTest.cpp b/vrs/os/test/EventUnitTest.cpp
@@ -52,7 +52,7 @@ struct EventTest : public testing::Test {
   void setupEvent(EventChannel::NotificationMode mode) {
     TearDown();
 
-    testEventChannel.reset(new EventChannel("TestEventChannel", mode));
+    testEventChannel = make_unique<EventChannel>("TestEventChannel", mode);
     EXPECT_NE(nullptr, testEventChannel);
   }
 
@@ -65,7 +65,7 @@ struct EventTest : public testing::Test {
   }
 
   void startWaitThread(const vector<double>& waitIntervals) {
-    waitThreads.push_back(make_unique<thread>(waitOnEvents, this, waitIntervals));
+    waitThreads.emplace_back(make_unique<thread>(waitOnEvents, this, waitIntervals));
   }
 
   void waitForLaunch() {
@@ -107,7 +107,7 @@ struct EventTest : public testing::Test {
   unique_ptr<thread> dispatchThread;
   vector<unique_ptr<thread>> waitThreads;
   vector<pair<double, void*>> eventParams;
-  EventChannel::Event event;
+  EventChannel::Event event{};
 };
 
 void dispatchEvents(void* data, bool synchronous) {

diff --git a/vrs/os/test/UtilsTest.cpp b/vrs/os/test/UtilsTest.cpp
@@ -33,11 +33,11 @@ using namespace std;
 using namespace vrs;
 
 struct FileTest : testing::Test {
-  FileTest() {}
+  FileTest() = default;
 
   string testDataDir = coretech::getTestDataDir();
 
-  void testFileName(const string& filename) {
+  static void testFileName(const string& filename) {
     string path = os::pathJoin(os::makeUniqueFolder(), os::sanitizeFileName(filename));
     int status = os::makeDir(path);
     if (status != 0) {
@@ -136,7 +136,7 @@ TEST_F(FileTest, testGetFileSize) {
 #endif // !IS_ANDROID_PLATFORM()
 
 struct GetCurrentExecutablePathTest : testing::Test {
-  GetCurrentExecutablePathTest() {}
+  GetCurrentExecutablePathTest() = default;
 
   string currentExectable = os::getCurrentExecutablePath();
 };

diff --git a/vrs/test/helpers/VRSTestsHelpers.cpp b/vrs/test/helpers/VRSTestsHelpers.cpp
@@ -66,12 +66,12 @@ class ForwardDiskFile : public DiskFile {
 
 class BlankStreamPlayer : public StreamPlayer {
  public:
-  virtual bool processRecordHeader(const CurrentRecord& record, DataReference& outDataReference) {
+  bool processRecordHeader(const CurrentRecord& record, DataReference& outDataReference) override {
     buffer_.resize(record.recordSize);
     outDataReference.useVector(buffer_);
     return true;
   }
-  virtual void processRecord(const CurrentRecord&, uint32_t) {}
+  void processRecord(const CurrentRecord&, uint32_t) override {}
   vector<char> buffer_;
 };
 
@@ -98,15 +98,15 @@ class DawnCamera : public Recordable {
     return createRecord(-1, Record::Type::STATE, kStateVersion);
   }
   void addStateRecord(deque<IndexRecord::DiskRecordInfo>& index) {
-    index.push_back({-1, 0, this->getStreamId(), Record::Type::STATE});
+    index.emplace_back(-1, 0, this->getStreamId(), Record::Type::STATE);
   }
 
   const Record* createConfigurationRecord() override {
     // "old" timestamp to force testing out of order records system!
     return createRecord(-2, Record::Type::CONFIGURATION, kConfigurationVersion);
   }
   void addConfigurationRecord(deque<IndexRecord::DiskRecordInfo>& index) {
-    index.push_back({-2, 0, this->getStreamId(), Record::Type::CONFIGURATION});
+    index.emplace_back(-2, 0, this->getStreamId(), Record::Type::CONFIGURATION);
   }
 
   const Record* createFrame(uint32_t frameNumber) {
@@ -125,18 +125,18 @@ class DawnCamera : public Recordable {
         DataSource(frameData_, buffer));
   }
   void addFrame(deque<IndexRecord::DiskRecordInfo>& index, uint32_t frameNumber) {
-    index.push_back(
-        {getFrameTime(frameNumber),
-         getSizeOfFrame(frameNumber),
-         this->getStreamId(),
-         Record::Type::DATA});
+    index.emplace_back(
+        getFrameTime(frameNumber),
+        getSizeOfFrame(frameNumber),
+        this->getStreamId(),
+        Record::Type::DATA);
   }
 
   uint32_t getIndex() const {
     return cameraIndex_;
   }
 
-  uint32_t getSizeOfFrame(uint32_t) const {
+  static uint32_t getSizeOfFrame(uint32_t) {
     return kFrameWidth * kFrameHeight;
   }
 
@@ -291,7 +291,7 @@ int threadedCreateRecords(CreateParams& p) {
   vector<thread> threads;
   threads.reserve(kCameraCount);
   for (uint32_t threadIndex = 0; threadIndex < kCameraCount; threadIndex++) {
-    threads.push_back(thread{&createRecordsThreadTask, &threadParams[threadIndex]});
+    threads.emplace_back(&createRecordsThreadTask, &threadParams[threadIndex]);
   }
   for (uint32_t threadIndex = 0; threadIndex < kCameraCount; threadIndex++) {
     XR_LOGD("Joining thread #{}", threadIndex);

diff --git a/vrs/test/helpers/VRSTestsHelpers.h b/vrs/test/helpers/VRSTestsHelpers.h
@@ -98,7 +98,7 @@ struct CreateParams {
   using CustomCreateFileFunction =
       function<int(CreateParams& createParams, RecordFileWriter& fileWriter)>;
 
-  CreateParams(const string& _path, const FileConfig& _fileConfig = kClassicFileConfig)
+  explicit CreateParams(const string& _path, const FileConfig& _fileConfig = kClassicFileConfig)
       : path{_path}, fileConfig{_fileConfig} {}
 
   // Required params
@@ -126,7 +126,8 @@ struct CreateParams {
     chunkHandler = std::move(handler);
     return *this;
   }
-  CreateParams& setCustomCreateFileFunction(CustomCreateFileFunction _customCreateFileFunction) {
+  CreateParams& setCustomCreateFileFunction(
+      const CustomCreateFileFunction& _customCreateFileFunction) {
     customCreateFileFunction = _customCreateFileFunction;
     return *this;
   }
@@ -155,7 +156,7 @@ int singleThreadCreateRecords(CreateParams& createParams);
 
 /// Parameterization of the checks to be performed, so we can verify a wide variety of situations.
 struct CheckParams {
-  CheckParams(const string& _path, const FileConfig& _fileConfig = kClassicFileConfig)
+  explicit CheckParams(const string& _path, const FileConfig& _fileConfig = kClassicFileConfig)
       : filePath{_path}, fileConfig{_fileConfig} {}
 
   // Required params