fix bugs and add .clang-tidy #4391
Conversation
Signed-off-by: daquexian <daquexian566@gmail.com>
c40d886 to
0ad799f
Compare
| Dimension() : is_unknown(true) {} | ||
| Dimension(std::string param) : is_unknown(false), is_int(false), dim(-1), param(std::move(param)) {} | ||
| Dimension(int64_t dim) : is_unknown(false), is_int(true), dim(dim) {} | ||
| Dimension() : is_unknown(true), is_int(false), dim(-1) {} |
There was a problem hiding this comment.
Critical bug: In C++, built-in types like is_int and dim will not be initialized unless we initialize them explicitly, so is_int and dim are in an uninitialized state even after calling Dimension(), and then accessing these two members is undefined behavior. It causes bugs in IR users like onnx optimizer and onnxsim (and also version converter, I think)
| @@ -1119,7 +1125,7 @@ struct Graph final { | |||
| // Adds to graph initializer list, initializer names list, and as a graph input | |||
| // Also syncs the initializer name, tensor name, and value name | |||
| // Create an initializer whose value is stored in input | |||
| Value* addInitializerAndInput(const Tensor& initializer, std::string name) { | |||
| Value* addInitializerAndInput(const Tensor& initializer, const std::string& name) { | |||
There was a problem hiding this comment.
Performance improvement: copying std::string may be expensive
| @@ -1172,7 +1178,7 @@ struct Graph final { | |||
|
|
|||
| friend std::ostream& operator<<(std::ostream& out, const Graph& g); | |||
|
|
|||
| void forSelfAndEachSubGraph(std::function<void(Graph*)> fn) { | |||
| void forSelfAndEachSubGraph(const std::function<void(Graph*)>& fn) { | |||
There was a problem hiding this comment.
Performance improvement: copying std::function may be expensive
| @@ -256,7 +256,7 @@ std::unique_ptr<Graph> graphProtoToGraph(const ONNX_NAMESPACE::GraphProto& gp, b | |||
| } | |||
|
|
|||
| for (int i = 0; i < gp.input_size(); i++) { | |||
| auto vip = gp.input(i); | |||
| const auto& vip = gp.input(i); | |||
There was a problem hiding this comment.
Performance improvement: avoid unnecessary copy of ValueInfoProto
onnx/common/tensor.h
Outdated
| @@ -75,6 +75,9 @@ struct Tensor final { | |||
| name_ = std::string(other.name_.data(), other.name_.size()); | |||
| raw_data_ = std::string(other.raw_data_.data(), other.raw_data_.size()); | |||
| } | |||
| Tensor(Tensor&&) = default; | |||
| Tensor& operator=(Tensor&&) = default; | |||
There was a problem hiding this comment.
Performance improvement: move constructor and move assignment operator are implicit deleted if copy constructor or copy assignment operator is explicitly declared, so they should be defined explicitly to enable moving.
| @@ -52,7 +52,10 @@ class ResourceGuard final { | |||
| bool released_; | |||
|
|
|||
| public: | |||
| ResourceGuard(std::function<void()> destructor) : destructor_(std::move(destructor)), released_(false) {} | |||
| ONNX_DISALLOW_COPY_AND_ASSIGN(ResourceGuard); | |||
There was a problem hiding this comment.
Bug: ResourceGuard should not be copy-able, for the destructor function can only be executed once.
| @@ -75,6 +75,8 @@ struct Tensor final { | |||
| name_ = std::string(other.name_.data(), other.name_.size()); | |||
| raw_data_ = std::string(other.raw_data_.data(), other.raw_data_.size()); | |||
| } | |||
| Tensor(Tensor&&) = default; | |||
There was a problem hiding this comment.
Performance improvement: move constructor and move assignment operator are implicit deleted if copy constructor or copy assignment operator is explicitly declared, so move constructor should be defined explicitly to enable moving.
| @@ -75,6 +75,8 @@ struct Tensor final { | |||
| name_ = std::string(other.name_.data(), other.name_.size()); | |||
| raw_data_ = std::string(other.raw_data_.data(), other.raw_data_.size()); | |||
| } | |||
| Tensor(Tensor&&) = default; | |||
| ~Tensor() = default; | |||
There was a problem hiding this comment.
Define destructor explicitly to meet the rule of five
| } else { | ||
| // a dimension that has neither dim_value nor dim_param set | ||
| // represents an unknown dimension unrelated to other unknown dimensions. | ||
| dims.push_back(Dimension()); | ||
| dims.emplace_back(); |
There was a problem hiding this comment.
Performance improvement: emplace_back(...) is more effective than push_back(Dimension(...)) because it constructs the Dimension object in-place so no copy occurs.
Signed-off-by: daquexian <daquexian566@gmail.com>
Signed-off-by: daquexian <daquexian566@gmail.com>
| - key: performance-move-const-arg.CheckTriviallyCopyableMove | ||
| value: False | ||
| - key: cppcoreguidelines-special-member-functions.AllowMissingMoveFunctionsWhenCopyIsDeleted | ||
| value: True |
There was a problem hiding this comment.
It looks useful. Do you think we should add this check into CI as well?
There was a problem hiding this comment.
@jcwchen sorry for the late response. Yes, it is definitely better if we have this in CI. Maybe we can add it into CI in another PR.
* fix bugs and add .clang-tidy Signed-off-by: daquexian <daquexian566@gmail.com> * reformat Signed-off-by: daquexian <daquexian566@gmail.com> * fix header file Signed-off-by: daquexian <daquexian566@gmail.com> * allow implicit conversion of Dimension Signed-off-by: daquexian <daquexian566@gmail.com> Co-authored-by: Chun-Wei Chen <jacky82226@gmail.com>
Description
Motivation and Context
An onnxsim user reports that onnxsim fails to simplify some models. After debugging I found that it is because the
boolandintmember ofDimensionclass in onnx IR is not initialized, so accessing them is undefined behavior.To avoid this kind of bug as possible, I created a .clang-tidy config file to enable clang-tidy in vscode/vim/... when using clangd, and fixed other bugs clang-tidy reported.