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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp:227
+    std::string MutableFieldName =
+        ("mutable_" + ProtoAddFieldCall->getMethodDecl()->getName().substr(4))
+            .str();
----------------
congliu wrote:
> hokein wrote:
> > nit: getName().drop_front(sizeof("add_")).
> Used 'sizeof("add_")-1' since "add_" is const char [5].
how about?

```
llvm::StringRef FieldName =
        ProtoAddFieldCall->getMethodDecl()->getName();
FieldName.consume_front("add_");
std::string MutableFieldName = ...;
```


================
Comment at: clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp:233
+      if (Matches.empty()) {
+        // There is no method with name "mutable_xxx".
+        return;
----------------
congliu wrote:
> 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.
I wasn't aware of this corner case, could you add a comment describing the heuristic we use here (around the matcher)?


================
Comment at: clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp:15
 #include "../utils/OptionsUtils.h"
+#include "third_party/llvm/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchers.h"
 
----------------
the include (probably added by clangd) is for google internal style, I believe you are developing at google internal codebase (rather than the opensource LLVM codebase)?


================
Comment at: clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp:192
 
+  const CXXMemberCallExpr *AppendCall = VectorAppendCall;
+  if (!AppendCall)
----------------
nit:  `const CXXMemberCallExpr *AppendCall =VectorAppendCall? VectorAppendCall:ProtoAddFieldCall;`


================
Comment at: clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp:195
+    AppendCall = ProtoAddFieldCall;
+  if (!AppendCall)
+    return;
----------------
In theory, this case should not be happened -- the callback is only invoked when we find a match for the interesting vector/proto cases. 

Just use `assert(AppendCall) << "no append call expression"`.


================
Comment at: clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp:263
+              << AppendCall->getMethodDecl()->getDeclName()
+              << (VectorAppendCall != nullptr ? "vector" : "repeated field");
+  if (!ReserveSize.empty()) {
----------------
I think the container name doesn't provide much information in the diag message, maybe just use 
`%0 is called inside a loop; consider pre-allocating the container capacity before the loop`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67135/new/

https://reviews.llvm.org/D67135