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

[Haojian Wu via Phabricator via cfe-commits]

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