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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
