congliu added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp:227
+ std::string MutableFieldName =
+ ("mutable_" + ProtoAddFieldCall->getMethodDecl()->getName().substr(4))
+ .str();
----------------
hokein wrote:
> nit: getName().drop_front(sizeof("add_")).
Used 'sizeof("add_")-1' since "add_" is const char [5].
================
Comment at:
clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp:233
+ if (Matches.empty()) {
+ // There is no method with name "mutable_xxx".
+ return;
----------------
hokein wrote:
> 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.
I intended to rule out the corner cases when a proto field name is "add_xxx".
But now I think checking whether there is a "mutable_xxx" method is not a
effective way to rule out the corner case. A simpler way is just checking
whether add_xxx is const... I have updated the matcher to exclude const methods.
================
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.
----------------
hokein wrote:
> 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)...
Good point. Let's avoid those false positives.
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
