Clang-tidy clean-ups (vrs core 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: D52719364 fbshipit-source-id: e45b5a02c5bfa03e0a7618b7ada117789df2ee67
facebookresearch · Jan 13, 2024 · 0959e24 · 0959e24
1 parent 04315c9
commit 0959e24
Show file tree

Hide file tree

Showing 65 changed files with 370 additions and 369 deletions.
diff --git a/vrs/Compressor.cpp b/vrs/Compressor.cpp
@@ -175,7 +175,7 @@ class Compressor::CompressorImpl {
       zstdContext_ = ZSTD_createCCtx();
     }
     int compressionLevel = sZstdPresets[zstdPreset];
-    size_t zresult;
+    size_t zresult = 0;
     IF_ZCOMP_ERROR_LOG_AND_RETURN(
         ZSTD_CCtx_setParameter(zstdContext_, ZSTD_c_compressionLevel, compressionLevel));
     IF_ZCOMP_ERROR_LOG_AND_RETURN(ZSTD_CCtx_setPledgedSrcSize(zstdContext_, dataSize));
@@ -188,7 +188,7 @@ class Compressor::CompressorImpl {
       uint32_t& inOutCompressedSize,
       size_t maxCompressedSize,
       bool endFrame) {
-    size_t zresult;
+    size_t zresult = 0;
     do {
       IF_ZCOMP_ERROR_LOG_AND_RETURN(ZSTD_compressStream2(
           zstdContext_, output, input, endFrame ? ZSTD_e_end : ZSTD_e_continue));
@@ -210,7 +210,7 @@ class Compressor::CompressorImpl {
   }
 
  private:
-  const LZ4F_preferences_t* getLz4Preferences(CompressionPreset lz4Preset) const {
+  static const LZ4F_preferences_t* getLz4Preferences(CompressionPreset lz4Preset) {
     static LZ4F_preferences_t sLz4FastPreset;
     static LZ4F_preferences_t sLz4TightPreset;
     static once_flag sPresetInitFlag;

diff --git a/vrs/ContentBlockReader.cpp b/vrs/ContentBlockReader.cpp
@@ -112,7 +112,7 @@ ContentBlockReader::~ContentBlockReader() = default;
 size_t ContentBlockReader::findContentBlockSize(
     const CurrentRecord& record,
     RecordFormatStreamPlayer& player) {
-  uint32_t size32;
+  uint32_t size32 = 0;
   // Have we successfully mapped the content block size already?
   if (contentBlockSizeSpec_ && contentBlockSizeSpec_->isMapped() &&
       contentBlockSizeSpec_->nextContentBlockSize.get(size32)) {
@@ -229,7 +229,7 @@ bool AudioBlockReader::findAudioSpec(
 
 bool AudioBlockReader::audioContentFromAudioSpec(
     const datalayout_conventions::AudioSpec& audioSpec,
-    ContentBlock& audioContentBlock) const {
+    ContentBlock& audioContentBlock) {
   AudioSampleFormat sampleFormat = AudioSampleFormat::UNDEFINED;
   uint8_t numChannels = 0;
   uint32_t sampleRate = 0;

diff --git a/vrs/ContentBlockReader.h b/vrs/ContentBlockReader.h
@@ -94,7 +94,7 @@ class AudioBlockReader : public ContentBlockReader {
 
  protected:
   bool readAudioContentBlock(const CurrentRecord&, RecordFormatStreamPlayer&, const ContentBlock&);
-  bool audioContentFromAudioSpec(const datalayout_conventions::AudioSpec&, ContentBlock&) const;
+  static bool audioContentFromAudioSpec(const datalayout_conventions::AudioSpec&, ContentBlock&);
   bool findAudioSpec(
       const CurrentRecord&,
       RecordFormatStreamPlayer&,

diff --git a/vrs/DataLayout.cpp b/vrs/DataLayout.cpp
@@ -599,8 +599,9 @@ DataPieceString* DataLayout::findDataPieceString(const string& label) {
       const_cast<const DataLayout*>(this)->findDataPieceString(label));
 }
 
-void DataLayout::forEachDataPiece(function<void(const DataPiece*)> callback, DataPieceType type)
-    const {
+void DataLayout::forEachDataPiece(
+    const function<void(const DataPiece*)>& callback,
+    DataPieceType type) const {
   if (type == DataPieceType::Undefined || type == DataPieceType::Value ||
       type == DataPieceType::Array) {
     for (const DataPiece* piece : fixedSizePieces_) {
@@ -618,7 +619,7 @@ void DataLayout::forEachDataPiece(function<void(const DataPiece*)> callback, Dat
   }
 }
 
-void DataLayout::forEachDataPiece(function<void(DataPiece*)> callback, DataPieceType type) {
+void DataLayout::forEachDataPiece(const function<void(DataPiece*)>& callback, DataPieceType type) {
   if (type == DataPieceType::Undefined || type == DataPieceType::Value ||
       type == DataPieceType::Array) {
     for (DataPiece* piece : fixedSizePieces_) {
@@ -795,19 +796,19 @@ void adjustPrecision<double>(const double& v, ostream& str) {
 // Print char/int8/uint8 as numbers
 // https://stackoverflow.com/questions/19562103/uint8-t-cant-be-printed-with-cout
 namespace special_chars {
-static ostream& operator<<(ostream& os, char c) {
+ostream& operator<<(ostream& os, char c) {
   return is_signed<char>::value ? os << static_cast<int>(c) : os << static_cast<unsigned int>(c);
 }
 
-static ostream& operator<<(ostream& os, signed char c) {
+ostream& operator<<(ostream& os, signed char c) {
   return os << static_cast<int>(c);
 }
 
-static ostream& operator<<(ostream& os, unsigned char c) {
+ostream& operator<<(ostream& os, unsigned char c) {
   return os << static_cast<unsigned int>(c);
 }
 
-static ostream& operator<<(ostream& os, const string& s) {
+ostream& operator<<(ostream& os, const string& s) {
   return std::operator<<(os, helpers::make_printable(s));
 }
 
@@ -1087,12 +1088,9 @@ bool DataPieceValue<T>::isSame(const DataPiece* rhs) const {
   if (!DataPiece::isSame(rhs)) {
     return false;
   }
-  const DataPieceValue<T>& other = *reinterpret_cast<const DataPieceValue<T>*>(rhs);
-  if (!vrs::isSame(this->defaultValue_, other.defaultValue_) ||
-      !vrs::isSame(this->properties_, other.properties_)) {
-    return false;
-  }
-  return true;
+  const auto* other = reinterpret_cast<const DataPieceValue<T>*>(rhs);
+  return vrs::isSame(this->defaultValue_, other->defaultValue_) &&
+      vrs::isSame(this->properties_, other->properties_);
 }
 
 template <typename T>
@@ -1196,12 +1194,9 @@ bool DataPieceArray<T>::isSame(const DataPiece* rhs) const {
   if (!DataPiece::isSame(rhs)) {
     return false;
   }
-  const DataPieceArray<T>& other = *reinterpret_cast<const DataPieceArray<T>*>(rhs);
-  if (!vrs::isSame(this->defaultValues_, other.defaultValues_) ||
-      !vrs::isSame(this->properties_, other.properties_)) {
-    return false;
-  }
-  return true;
+  const auto* other = reinterpret_cast<const DataPieceArray<T>*>(rhs);
+  return vrs::isSame(this->defaultValues_, other->defaultValues_) &&
+      vrs::isSame(this->properties_, other->properties_);
 }
 
 template <typename T>
@@ -1272,9 +1267,9 @@ size_t DataPieceVector<string>::collectVariableData(int8_t* data, size_t bufferS
 
 template <>
 bool DataPieceVector<string>::get(vector<string>& outValues) const {
-  size_t byteCount;
+  size_t byteCount = 0;
   const int8_t* source = layout_.getVarData<int8_t>(offset_, byteCount);
-  uint32_t vectorSize;
+  uint32_t vectorSize = 0;
   size_t readSize = 0;
   if (loadElement<uint32_t>(vectorSize, source, readSize, byteCount)) {
     if ((vectorSize + 1) * sizeof(uint32_t) <= byteCount) {
@@ -1456,7 +1451,7 @@ size_t DataPieceStringMap<T>::collectVariableData(int8_t* data, size_t bufferSiz
 template <typename T>
 bool DataPieceStringMap<T>::get(map<string, T>& outValues) const {
   outValues.clear();
-  size_t dataSize;
+  size_t dataSize = 0;
   const int8_t* ptr = layout_.getVarData<int8_t>(offset_, dataSize);
   size_t readSize = 0;
   if (ptr != nullptr && dataSize > 0) {
@@ -1530,11 +1525,8 @@ bool DataPieceStringMap<T>::isSame(const DataPiece* rhs) const {
   if (!DataPiece::isSame(rhs)) {
     return false;
   }
-  const DataPieceStringMap<T>& other = *reinterpret_cast<const DataPieceStringMap<T>*>(rhs);
-  if (!vrs::isSame(this->defaultValues_, other.defaultValues_)) {
-    return false;
-  }
-  return true;
+  const auto* other = reinterpret_cast<const DataPieceStringMap<T>*>(rhs);
+  return vrs::isSame(this->defaultValues_, other->defaultValues_);
 }
 
 template <typename T>
@@ -1578,13 +1570,13 @@ size_t DataPieceString::collectVariableData(int8_t* data, size_t bufferSize) con
 }
 
 string DataPieceString::get() const {
-  size_t size;
+  size_t size = 0;
   const char* ptr = layout_.getVarData<char>(offset_, size);
   return ptr != nullptr ? string(ptr, size) : defaultString_;
 }
 
 bool DataPieceString::get(string& outString) const {
-  size_t size;
+  size_t size = 0;
   const char* ptr = layout_.getVarData<char>(offset_, size);
   if (ptr != nullptr) {
     outString.resize(0);
@@ -1596,7 +1588,7 @@ bool DataPieceString::get(string& outString) const {
 }
 
 bool DataPieceString::isAvailable() const {
-  size_t count;
+  size_t count = 0;
   return layout_.getVarData<char>(offset_, count) != nullptr;
 }
 

diff --git a/vrs/DataLayout.h b/vrs/DataLayout.h
@@ -92,8 +92,8 @@ struct JsonFormatProfileSpec {
   bool required = true; ///< Include the required flag.
 
   // Default format
-  JsonFormatProfileSpec() {}
-  JsonFormatProfileSpec(JsonFormatProfile profile);
+  JsonFormatProfileSpec() = default;
+  explicit JsonFormatProfileSpec(JsonFormatProfile profile);
 };
 
 /// \brief The DataLayout class describes the data stored inside a DataLayoutContentBlock.
@@ -188,10 +188,11 @@ struct JsonFormatProfileSpec {
 class DataLayout {
  protected:
   DataLayout() = default;
+
+ public:
   DataLayout& operator=(const DataLayout&) = delete;
   DataLayout(const DataLayout&) = delete;
 
- public:
   /// DataLayout has no virtual method, but it is used in containers, and some of its
   /// derived classes have important clean-up work to do in their destructor.
   /// Therefore, DataLayout requires a virtual destructor.
@@ -395,11 +396,11 @@ class DataLayout {
   /// @param callback: a function to call for each element found.
   /// @param type: filter to select only an element type, of UNDEFINED for no filtering.
   void forEachDataPiece(
-      std::function<void(const DataPiece*)>,
+      const std::function<void(const DataPiece*)>&,
       DataPieceType type = DataPieceType::Undefined) const;
   /// Same as above, but as a non-const version.
   void forEachDataPiece(
-      std::function<void(DataPiece*)>,
+      const std::function<void(DataPiece*)>&,
       DataPieceType type = DataPieceType::Undefined);
 
   /// For debugging: validate that the index for the variable size data looks valid.
@@ -501,7 +502,7 @@ class DataLayout {
   /// Buffer to hold fixed-size pieces, and the index of var size pieces (if any).
   vector<int8_t> fixedData_;
   /// Byte count for all the fixed size pieces + var size index.
-  size_t fixedDataSizeNeeded_;
+  size_t fixedDataSizeNeeded_{};
   /// Buffer holding variable-size pieces, after they've been collected, or read from disk.
   vector<int8_t> varData_;
   /// Tells all the required pieces have been mapped successfully.
@@ -561,7 +562,7 @@ class ManualDataLayout : public DataLayout {
   ManualDataLayout();
   /// For manual construction, based on an existing layout, cloning all the pieces.
   /// Add more pieces using "add()", but don't forget to call endLayout() when you're done.
-  ManualDataLayout(const DataLayout& layout);
+  explicit ManualDataLayout(const DataLayout& layout);
 
   ~ManualDataLayout() override;
 
@@ -613,19 +614,21 @@ class ManualDataLayout : public DataLayout {
 /// Use the alternate macro DATA_LAYOUT_STRUCT_WITH_INIT if you need your DataLayoutStruct to be
 /// initialized by an init() method, to assign default values to DataPiece fields for instance.
 struct DataLayoutStruct {
-  DataLayoutStruct(const string& structName);
-  void dataLayoutStructEnd(const string& structName);
+  explicit DataLayoutStruct(const string& structName);
+  static void dataLayoutStructEnd(const string& structName);
 };
 
-#define DATA_LAYOUT_STRUCT(DATA_LAYOUT_STRUCT_TYPE)                                           \
-  DATA_LAYOUT_STRUCT_TYPE(const std::string& _structName_) : DataLayoutStruct(_structName_) { \
-    dataLayoutStructEnd(_structName_);                                                        \
+#define DATA_LAYOUT_STRUCT(DATA_LAYOUT_STRUCT_TYPE)                 \
+  explicit DATA_LAYOUT_STRUCT_TYPE(const std::string& _structName_) \
+      : DataLayoutStruct(_structName_) {                            \
+    dataLayoutStructEnd(_structName_);                              \
   }
 
-#define DATA_LAYOUT_STRUCT_WITH_INIT(DATA_LAYOUT_STRUCT_TYPE)                                 \
-  DATA_LAYOUT_STRUCT_TYPE(const std::string& _structName_) : DataLayoutStruct(_structName_) { \
-    dataLayoutStructEnd(_structName_);                                                        \
-    init();                                                                                   \
+#define DATA_LAYOUT_STRUCT_WITH_INIT(DATA_LAYOUT_STRUCT_TYPE)       \
+  explicit DATA_LAYOUT_STRUCT_TYPE(const std::string& _structName_) \
+      : DataLayoutStruct(_structName_) {                            \
+    dataLayoutStructEnd(_structName_);                              \
+    init();                                                         \
   }
 
 /// \brief Helper class to include DataLayout structs containing a sliced array of DataPieceXXX and
@@ -685,7 +688,7 @@ struct DataLayoutStructArray : public vrs::DataLayoutStruct {
 template <class OptionalFields>
 class OptionalDataPieces : public std::unique_ptr<OptionalFields> {
  public:
-  OptionalDataPieces(bool allocateFields)
+  explicit OptionalDataPieces(bool allocateFields)
       : std::unique_ptr<OptionalFields>(
             allocateFields ? std::make_unique<OptionalFields>() : nullptr) {}
 };

diff --git a/vrs/DataLayoutConventions.cpp b/vrs/DataLayoutConventions.cpp
@@ -60,14 +60,14 @@ ContentBlock ImageSpec::getImageContentBlock(const ImageContentBlockSpec& base,
     uint32_t readStride = stride.get(); // get value or 0
     uint32_t readStride2 = stride2.get(); // get value or 0
     if (base.getImageFormat() == ImageFormat::RAW) {
-      return ContentBlock(readPixelFormat, readWidth, readHeight, readStride, readStride2);
+      return {readPixelFormat, readWidth, readHeight, readStride, readStride2};
     } else if (base.getImageFormat() == ImageFormat::VIDEO) {
       if (blockSize != ContentBlock::kSizeUnknown) {
         string aCodecName;
         if (!codecName.get(aCodecName) || aCodecName.empty()) {
           aCodecName = base.getCodecName();
         }
-        uint32_t aCodecQuality;
+        uint32_t aCodecQuality = 0;
         if (!codecQuality.get(aCodecQuality) ||
             !ImageContentBlockSpec::isQualityValid(aCodecQuality)) {
           aCodecQuality = base.getCodecQuality();

diff --git a/vrs/DataPieceArray.h b/vrs/DataPieceArray.h
@@ -52,7 +52,7 @@ class DataPieceArray : public DataPiece {
   }
   /// @param bundle: Bundle to reconstruct a DataPieceArray from disk.
   /// @internal
-  DataPieceArray(const MakerBundle& bundle);
+  explicit DataPieceArray(const MakerBundle& bundle);
 
   /// Get the size of the array.
   size_t getArraySize() const {
@@ -327,7 +327,7 @@ class DataPieceArray : public DataPiece {
   }
 
  private:
-  const size_t count_;
+  const size_t count_{};
   map<string, T> properties_;
   vector<T> defaultValues_;
 };

diff --git a/vrs/DataPieceString.h b/vrs/DataPieceString.h
@@ -36,11 +36,11 @@ using std::string;
 class DataPieceString : public DataPiece {
  public:
   /// @param label: Name for the DataPiece.
-  DataPieceString(const string& label)
+  explicit DataPieceString(const string& label)
       : DataPiece(label, DataPieceType::String, DataLayout::kVariableSize) {}
   /// @param bundle: Bundle to reconstruct a DataPieceString from disk.
   /// @internal
-  DataPieceString(const MakerBundle& bundle);
+  explicit DataPieceString(const MakerBundle& bundle);
 
   /// Get the DataPiece element type name.
   /// @return "string".

diff --git a/vrs/DataPieceStringMap.h b/vrs/DataPieceStringMap.h
@@ -39,11 +39,11 @@ template <typename T>
 class DataPieceStringMap : public DataPiece {
  public:
   /// @param label: Name for the DataPiece.
-  DataPieceStringMap(const string& label)
+  explicit DataPieceStringMap(const string& label)
       : DataPiece(label, DataPieceType::StringMap, DataLayout::kVariableSize) {}
   /// @param bundle: Bundle to reconstruct a DataPieceStringMap from disk.
   /// @internal
-  DataPieceStringMap(const MakerBundle& bundle);
+  explicit DataPieceStringMap(const MakerBundle& bundle);
 
   /// Get the name of the element type <T>.
   /// @internal
@@ -104,7 +104,7 @@ class DataPieceStringMap : public DataPiece {
   /// Tell if the DataPiece is available, directly or mapped successfully.
   /// @return True if values can be read without using default values.
   bool isAvailable() const override {
-    size_t count;
+    size_t count = 0;
     return layout_.getVarData<int8_t>(offset_, count) != nullptr;
   }
 

diff --git a/vrs/DataPieceTypes.h b/vrs/DataPieceTypes.h
@@ -103,10 +103,11 @@ struct PointND {
   bool operator!=(const PointND<T, N>& rhs) const {
     return !operator==(rhs);
   }
-  void operator=(const T rhs[N]) {
+  PointND& operator=(const T rhs[N]) {
     for (size_t s = 0; s < N; s++) {
       dim[s] = rhs[s];
     }
+    return *this;
   }
 
   // Convenience aliases for the dimensions that actually exist
@@ -176,7 +177,7 @@ struct MatrixND {
   using type = T;
   static constexpr size_t kMatrixSize = N;
 
-  MatrixND() {}
+  MatrixND() = default;
   MatrixND(const T arr[N][N]) {
     operator=(arr);
   }
@@ -199,10 +200,11 @@ struct MatrixND {
   bool operator!=(const MatrixND<T, N>& rhs) const {
     return !operator==(rhs);
   }
-  void operator=(const T rhs[N][N]) {
+  MatrixND& operator=(const T rhs[N][N]) {
     for (size_t s = 0; s < N; s++) {
       points[s] = rhs[s];
     }
+    return *this;
   }
 
   PointND<T, N> points[N];