hokein added a comment.
your patch doesn't have a full context, please upload the patch with full
context (`=U99999`) or use Arcanist
<https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line>.
> I'd think the check instead should be parametrized, so this patch should
> become just extending the config.
The AST for protobuf is quite different compared with normal vector, so I don't
think there will be an easy and simple way to do it in a config way.
================
Comment at:
clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp:86
utils::options::serializeStringList(VectorLikeClasses));
+ Options.store(Opts, "EnableProto", static_cast<int64_t>(EnableProto));
}
----------------
nit: remove `static_cast`?
================
Comment at:
clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp:227
+ std::string MutableFieldName =
+ ("mutable_" + ProtoAddFieldCall->getMethodDecl()->getName().substr(4))
+ .str();
----------------
nit: getName().drop_front(sizeof("add_")).
================
Comment at:
clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp:233
+ if (Matches.empty()) {
+ // There is no method with name "mutable_xxx".
+ return;
----------------
for repeated fields, there should be `add_foo`, `mutable_foo` methods in the
proto generated code
(https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#repeatednumeric).
So I think we can remove this check here.
================
Comment at:
clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp:249
+ match.getNodeAs<CXXMemberCallExpr>("maybe_reallocation");
+ // Skip cases where "mutable_xxx" or "add_xxx" is called before the
+ // loop.
----------------
the heuristic is limited, and will fail the cases like below:
```
MyProto proto;
set_proto_xxx_size(&proto);
for (int i = 0; i < n; ++i) {
proto.add_xxx(i);
}
```
In the vector case, we do a more strict check, maybe we do the same way as well
(but it will make the check fail to spot some cases)...
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67135/new/
https://reviews.llvm.org/D67135
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
