Fix clang-tidy warnings (onnx#6378)

### Description  This PR fixes more clang-tidy warnings and adds more checks. ### Motivation and Context  Better code.  Signed-off-by: cyy <[email protected]>
AlexandreEichenberger · Sep 21, 2024 · 0649a3c · 0649a3c
1 parent 8f38dcb
commit 0649a3c
Show file tree

Hide file tree

Showing 21 changed files with 70 additions and 70 deletions.
diff --git a/.clang-tidy b/.clang-tidy
@@ -17,6 +17,7 @@ Checks: >-
   clang-analyzer-alpha.security.*,
   cppcoreguidelines-avoid-goto,
   cppcoreguidelines-interfaces-global-init,
+  cppcoreguidelines-init-variables,
   cppcoreguidelines-no-malloc,
   cppcoreguidelines-prefer-member-initializer,
   cppcoreguidelines-pro-type-member-init,
@@ -33,6 +34,13 @@ Checks: >-
   google-default-arguments,
   google-global-names-in-headers,
   google-explicit-constructor,
+  modernize-make-shared,
+  modernize-make-unique,
+  modernize-pass-by-value,
+  modernize-use-equals-default,
+  modernize-use-equals-delete,
+  modernize-use-nullptr,
+  modernize-use-override,
   modernize-use-emplace
 
 CheckOptions:

diff --git a/onnx/checker.h b/onnx/checker.h
@@ -10,11 +10,8 @@
 #include <unordered_set>
 #include <utility>
 
-#include "onnx/defs/function.h"
 #include "onnx/defs/schema.h"
 #include "onnx/onnx-data_pb.h"
-#include "onnx/onnx-operators_pb.h"
-#include "onnx/onnx_pb.h"
 #include "onnx/string_utils.h"
 
 namespace ONNX_NAMESPACE {
@@ -107,6 +104,7 @@ class CheckerContext final {
 class LexicalScopeContext final {
  public:
   LexicalScopeContext() = default;
+  ~LexicalScopeContext() = default;
 
   // Construct an instance with the lexical scope from the parent graph to allow
   // lookup of names from that scope via this_or_ancestor_graph_has.
@@ -119,6 +117,8 @@ class LexicalScopeContext final {
     parent_context_ = &parent_context;
     return *this;
   }
+  LexicalScopeContext(LexicalScopeContext&&) = delete;
+  LexicalScopeContext& operator=(LexicalScopeContext&&) = delete;
 
   void add(const std::string& name) {
     output_names.insert(name);

diff --git a/onnx/common/ir.h b/onnx/common/ir.h
@@ -132,10 +132,10 @@ struct ScalarAttributeValue final : public AttributeValue {
   ValueType& value() {
     return value_;
   }
-  virtual Ptr clone() const override {
+  Ptr clone() const override {
     return std::make_unique<ScalarAttributeValue>(name, value_);
   }
-  virtual AttributeKind kind() const override {
+  AttributeKind kind() const override {
     return Kind;
   }
 
@@ -147,14 +147,14 @@ template <typename T, AttributeKind Kind>
 struct VectorAttributeValue final : public AttributeValue {
   using ConstructorType = const std::vector<T>&&;
   using ValueType = std::vector<T>;
-  VectorAttributeValue(Symbol name, ConstructorType value_) : AttributeValue(name), value_(std::move(value_)) {}
+  VectorAttributeValue(Symbol name, ValueType value_) : AttributeValue(name), value_(std::move(value_)) {}
   ValueType& value() {
     return value_;
   }
-  virtual AttributeKind kind() const override {
+  AttributeKind kind() const override {
     return Kind;
   }
-  virtual std::unique_ptr<AttributeValue> clone() const override {
+  std::unique_ptr<AttributeValue> clone() const override {
     return std::make_unique<VectorAttributeValue>(name, ValueType(value_));
   }
 
@@ -181,7 +181,7 @@ using TypeProtosAttr = VectorAttributeValue<TypeProto, AttributeKind::tps>;
 // we return Derived* pointers because Nodes are normally held as pointers.
 template <typename Derived>
 struct Attributes {
-  Attributes() {}
+  Attributes() = default;
   void copyAttributes(const Attributes& rhs) {
     values_.clear();
     values_.reserve(rhs.values_.size());
@@ -375,7 +375,7 @@ struct Value final {
   Graph* owningGraph();
   const Graph* owningGraph() const;
   // TODO: make this more const correct
-  const use_list uses() const;
+  use_list uses() const;
 
   // Replaces all uses of this node with 'newValue'.
   //
@@ -839,7 +839,7 @@ class OpSetID final {
   // Default Domain Constructor
   explicit OpSetID(const int64_t version) : domain_(""), version_(version) {}
 
-  explicit OpSetID(const std::string& domain, int64_t version) : domain_(domain), version_(version) {}
+  explicit OpSetID(std::string domain, int64_t version) : domain_(std::move(domain)), version_(version) {}
 
   // target must be in the form "<domain>&<version>"
   std::string toString() const {
@@ -1425,7 +1425,7 @@ inline const_graph_node_list_iterator Node::reverseIterator() const {
 // nodes in subgraph are also included.
 // This method is usually used to check whether it is
 // safe to delete a Value.
-inline const use_list Value::uses() const {
+inline use_list Value::uses() const {
   use_list all_uses = uses_in_current_graph_;
   owningGraph()->forEachNode([this, &all_uses](const Node* node) {
     if (node->owningGraph() == this->owningGraph()) {

diff --git a/onnx/common/ir_pb_converter.cc b/onnx/common/ir_pb_converter.cc
@@ -227,7 +227,7 @@ void createDummyValue(
 }
 
 std::unique_ptr<Graph> graphProtoToGraph(const ONNX_NAMESPACE::GraphProto& gp, bool nested, const int ir_version) {
-  std::unique_ptr<Graph> g(new Graph());
+  auto g = std::make_unique<Graph>();
 
   if (gp.has_name()) {
     g->setName(gp.name());

diff --git a/onnx/common/model_helpers.cc b/onnx/common/model_helpers.cc
@@ -14,7 +14,7 @@ Common::Status BuildNode(
     std::vector<std::string> const& inputs,
     std::vector<std::string> const& outputs,
     NodeProto* node) {
-  if (node == NULL) {
+  if (node == nullptr) {
     return Common::Status(Common::CHECKER, Common::INVALID_ARGUMENT, "node_proto should not be nullptr.");
   }
   node->set_name(name);

diff --git a/onnx/common/status.cc b/onnx/common/status.cc
@@ -8,20 +8,22 @@
 
 #include <assert.h>
 
+#include <memory>
+
 #include "onnx/string_utils.h"
 
 namespace ONNX_NAMESPACE {
 namespace Common {
 
 Status::Status(StatusCategory category, int code, const std::string& msg) {
   assert(static_cast<int>(StatusCode::OK) != code);
-  state_.reset(new State(category, code, msg));
+  state_ = std::make_unique<State>(category, code, msg);
 }
 
 Status::Status(StatusCategory category, int code) : Status(category, code, EmptyString()) {}
 
 bool Status::IsOK() const noexcept {
-  return (state_ == NULL);
+  return (state_ == nullptr);
 }
 
 StatusCategory Status::Category() const noexcept {

diff --git a/onnx/common/status.h b/onnx/common/status.h
@@ -44,7 +44,7 @@ class Status {
       if (nullptr == other.state_) {
         state_.reset();
       } else if (state_ != other.state_) {
-        state_.reset(new State(*other.state_));
+        state_ = std::make_unique<State>(*other.state_);
       }
     }
   }

diff --git a/onnx/defs/data_type_utils.cc b/onnx/defs/data_type_utils.cc
@@ -186,8 +186,7 @@ void DataTypeUtils::FromString(const std::string& type_str, TypeProto& type_prot
     s.LStrip(key_size);
     s.LStrip(",");
     StringRange v(s.Data(), s.Size());
-    int32_t key_type;
-    FromDataTypeString(key, key_type);
+    auto key_type = FromDataTypeString(key);
     type_proto.mutable_map_type()->set_key_type(key_type);
     return FromString(std::string(v.Data(), v.Size()), *type_proto.mutable_map_type()->mutable_value_type());
   } else
@@ -211,18 +210,15 @@ void DataTypeUtils::FromString(const std::string& type_str, TypeProto& type_prot
 #endif
       if (s.LStrip("sparse_tensor")) {
     s.ParensWhitespaceStrip();
-    int32_t e;
-    FromDataTypeString(std::string(s.Data(), s.Size()), e);
+    auto e = FromDataTypeString(std::string(s.Data(), s.Size()));
     type_proto.mutable_sparse_tensor_type()->set_elem_type(e);
   } else if (s.LStrip("tensor")) {
     s.ParensWhitespaceStrip();
-    int32_t e;
-    FromDataTypeString(std::string(s.Data(), s.Size()), e);
+    auto e = FromDataTypeString(std::string(s.Data(), s.Size()));
     type_proto.mutable_tensor_type()->set_elem_type(e);
   } else {
     // Scalar
-    int32_t e;
-    FromDataTypeString(std::string(s.Data(), s.Size()), e);
+    auto e = FromDataTypeString(std::string(s.Data(), s.Size()));
     TypeProto::Tensor* t = type_proto.mutable_tensor_type();
     t->set_elem_type(e);
     // Call mutable_shape() to initialize a shape with no dimension.
@@ -236,14 +232,14 @@ bool DataTypeUtils::IsValidDataTypeString(const std::string& type_str) {
   return (allowedSet.find(type_str) != allowedSet.end());
 }
 
-void DataTypeUtils::FromDataTypeString(const std::string& type_str, int32_t& tensor_data_type) {
+int32_t DataTypeUtils::FromDataTypeString(const std::string& type_str) {
   if (!IsValidDataTypeString(type_str)) {
     ONNX_THROW_EX(std::invalid_argument(
         "DataTypeUtils::FromDataTypeString - Received invalid data type string '" + type_str + "'."));
   }
 
   TypesWrapper& t = TypesWrapper::GetTypesWrapper();
-  tensor_data_type = t.TypeStrToTensorDataType()[type_str];
+  return t.TypeStrToTensorDataType()[type_str];
 }
 
 StringRange::StringRange() : data_(""), size_(0), start_(data_), end_(data_) {}

diff --git a/onnx/defs/data_type_utils.h b/onnx/defs/data_type_utils.h
@@ -52,7 +52,7 @@ class DataTypeUtils final {
  private:
   static void FromString(const std::string& type_str, TypeProto& type_proto);
 
-  static void FromDataTypeString(const std::string& type_str, int32_t& tensor_data_type);
+  static int32_t FromDataTypeString(const std::string& type_str);
 
   static std::string ToString(const TypeProto& type_proto, const std::string& left = "", const std::string& right = "");
 

diff --git a/onnx/defs/function.h b/onnx/defs/function.h
@@ -29,7 +29,7 @@ class FunctionBodyHelper {
 
     AttributeProtoWrapper() = default;
 
-    AttributeProtoWrapper(const AttributeProto& attr_prot) : proto(attr_prot) {}
+    AttributeProtoWrapper(AttributeProto attr_prot) : proto(std::move(attr_prot)) {}
 
     template <typename T>
     AttributeProtoWrapper(const std::string& attr_name, const T& value) : proto(MakeAttribute(attr_name, value)) {}

diff --git a/onnx/defs/math/old.cc b/onnx/defs/math/old.cc
@@ -3579,8 +3579,8 @@ ONNX_OPERATOR_SET_SCHEMA(
 
           // Check for compatible matrix multiply dimensions
           {
-            auto dimL = shapeL.dim(shapeL.dim_size() - 1);
-            auto dimR = shapeR.dim(shapeR.dim_size() - 2);
+            auto const& dimL = shapeL.dim(shapeL.dim_size() - 1);
+            auto const& dimR = shapeR.dim(shapeR.dim_size() - 2);
             if (dimL.has_dim_value() && dimR.has_dim_value() && dimL.dim_value() != dimR.dim_value()) {
               fail_shape_inference("Incompatible dimensions for matrix multiplication");
               ;

diff --git a/onnx/defs/nn/defs.cc b/onnx/defs/nn/defs.cc
@@ -1215,7 +1215,7 @@ void convTransposeShapeInference(InferenceContext& ctx) {
       ctx.getInputType(1)->tensor_type().shape().dim(1) * group; // channels should be the second dim of second input
                                                                  // multiply group.
 
-  int size_of_output;
+  int size_of_output = 0;
   if (output_shape_presented) {
     size_of_output = static_cast<int>(output_shape.size());
     for (int i = 0; i < size_of_output; ++i) {

diff --git a/onnx/defs/nn/old.cc b/onnx/defs/nn/old.cc
@@ -251,7 +251,7 @@ void convPoolShapeInference_opset19(
 
     // how many times we can move the kernel from it's initial position, based
     // on the stride
-    int64_t strided_kernel_positions;
+    int64_t strided_kernel_positions = 0;
 
     if (ceil_mode == 1)
       strided_kernel_positions =
@@ -726,7 +726,7 @@ void convTransposeShapeInference_opset11(InferenceContext& ctx) {
       ctx.getInputType(1)->tensor_type().shape().dim(1) * group; // channels should be the second dim of second input
                                                                  // multiply group.
 
-  int size_of_output;
+  int size_of_output = 0;
   if (output_shape_presented) {
     size_of_output = static_cast<int>(output_shape.size());
     for (int i = 0; i < size_of_output; ++i) {
@@ -2013,7 +2013,7 @@ void convPoolShapeInference1(
 
     // how many times we can move the kernel from it's initial position, based
     // on the stride
-    int64_t strided_kernel_positions;
+    int64_t strided_kernel_positions = 0;
 
     if (ceil_mode == 1)
       strided_kernel_positions =
@@ -3004,7 +3004,7 @@ void convTransposeShapeInference1(InferenceContext& ctx) {
       ctx.getInputType(1)->tensor_type().shape().dim(1) * group; // channels should be the second dim of second input
                                                                  // multiply group.
 
-  int size_of_output;
+  int size_of_output = 0;
   if (output_shape_presented) {
     size_of_output = static_cast<int>(output_shape.size());
     for (int i = 0; i < size_of_output; ++i) {

diff --git a/onnx/defs/parser.cc b/onnx/defs/parser.cc
@@ -444,7 +444,7 @@ Status OnnxParser::Parse(TensorProto& tensorProto, const TypeProto& tensorTypePr
   // tensorProto.mutable_int64_data()->Reserve(n);
   // Parse the actual values:
 
-  int64_t intval;
+  int64_t intval = 0;
   uint64_t uintval = 0;
   float floatval = 0.0;
   double dblval = 0.0;
@@ -833,7 +833,7 @@ Status OnnxParser::Parse(OpsetIdList& opsets) {
 Status OnnxParser::Parse(ModelProto& model) {
   model.Clear();
   std::string strval;
-  int64_t intval;
+  int64_t intval = 0;
   if (Matches('<')) {
     do {
       KeyWordMap::KeyWord keyword = KeyWordMap::KeyWord::NONE;

diff --git a/onnx/defs/parser.h b/onnx/defs/parser.h
@@ -9,8 +9,6 @@
 
 #include <ctype.h>
 
-#include <iostream>
-#include <stdexcept>
 #include <string>
 #include <unordered_map>
 

diff --git a/onnx/defs/printer.cc b/onnx/defs/printer.cc
@@ -5,7 +5,6 @@
 #include "onnx/defs/printer.h"
 
 #include <iomanip>
-#include <vector>
 
 #include "onnx/defs/tensor_proto_util.h"
 

diff --git a/onnx/defs/schema.cc b/onnx/defs/schema.cc
@@ -247,7 +247,7 @@ void OpSchema::Verify(const NodeProto& node) const {
     };
 
     const auto& search = attributes_.find(name);
-    AttributeProto::AttributeType expected_type;
+    AttributeProto::AttributeType expected_type{};
     if (search != attributes_.end()) {
       expected_type = search->second.type;
     } else if (allows_unchecked_attributes_ || isInternalSymbol(name)) {
@@ -802,7 +802,7 @@ OpSchema& OpSchema::FunctionBody(const char* func_body, int opset_version) {
   if (opset_version == OpSchema::kUninitializedSinceVersion && since_version_ != OpSchema::kUninitializedSinceVersion) {
     opset_version = since_version_;
   }
-  std::shared_ptr<FunctionProto> function_proto(new FunctionProto());
+  auto function_proto = std::make_shared<FunctionProto>();
   OnnxParser parser(func_body);
   auto status = parser.Parse(*function_proto->mutable_node());
   if (!status.IsOK())
@@ -822,7 +822,7 @@ OpSchema& OpSchema::FunctionBody(const std::vector<NodeProto>& func_nodes, int o
   if (opset_version == OpSchema::kUninitializedSinceVersion && since_version_ != OpSchema::kUninitializedSinceVersion) {
     opset_version = since_version_;
   }
-  std::shared_ptr<FunctionProto> function_proto(new FunctionProto());
+  auto function_proto = std::make_shared<FunctionProto>();
   for (const auto& node : func_nodes) {
     auto new_node = function_proto->add_node();
     new_node->CopyFrom(node);
@@ -831,7 +831,7 @@ OpSchema& OpSchema::FunctionBody(const std::vector<NodeProto>& func_nodes, int o
   // opset import may have been set
   // we may need to update its version with the specified opset_version
   UpdateFunctionProtoOpsetImportVersion(*function_proto, opset_version);
-  opset_version_to_function_body_.emplace(opset_version, function_proto);
+  opset_version_to_function_body_.emplace(opset_version, std::move(function_proto));
   return *this;
 }
 
@@ -843,7 +843,7 @@ OpSchema& OpSchema::FunctionBody(
     opset_version = since_version_;
   }
 
-  std::shared_ptr<FunctionProto> function_proto(new FunctionProto());
+  auto function_proto = std::make_shared<FunctionProto>();
   for (auto& relied_opset : relied_opsets) {
     *(function_proto->mutable_opset_import()->Add()) = relied_opset;
   }
@@ -855,7 +855,7 @@ OpSchema& OpSchema::FunctionBody(
   // opset import may have been set
   // we may need to update its version with the specified opset_version
   UpdateFunctionProtoOpsetImportVersion(*function_proto, opset_version);
-  opset_version_to_function_body_.emplace(opset_version, function_proto);
+  opset_version_to_function_body_.emplace(opset_version, std::move(function_proto));
   return *this;
 }