[PATCH] D67135: [clang-tidy] performance-inefficient-vector-operation: Support proto repeated field

Cong Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Sep 6 14:42:15 PDT 2019


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





More information about the cfe-commits mailing list