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

[congliu]

Summary

Finds calls that add element to protobuf repeated field in a loop without calling Reserve() before the loop. Calling Reserve() first can avoid unnecessary memory reallocations.

A new option EnableProto is added to guard this feature.

[Eugene.Zelenko]

Please mention new option in Release Notes.

clang-tools-extra/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst

9

See other checks documentation as example of options descriptions.

[lebedev.ri]

I'm not sure this is the correct approach.
I'd think the check instead should be parametrized, so this patch should become just extending the config.

[congliu]

Updated option descriptions.

[congliu]

In D67135#1657034, @lebedev.ri wrote:

I'm not sure this is the correct approach.
I'd think the check instead should be parametrized, so this patch should become just extending the config.

Could you elaborate how the parameterized check will look like? By config did you mean the check options?

[congliu]

Uploaded the correct diff...

[hokein]

your patch doesn't have a full context, please upload the patch with full context (=U99999) or use Arcanist.

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.

clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp

86 ↗

(On Diff #218789)

nit: remove static_cast?

227 ↗

(On Diff #218789)

nit: getName().drop_front(sizeof("add_")).

233 ↗

(On Diff #218789)

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.

249 ↗

(On Diff #218789)

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)...

[congliu]

Addressed Haojian's comments.

• Do not warn when there is reference to the proto variable between its declaration and the loop body.
• Exclude const methods.
• Update tests.

clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp

227 ↗

(On Diff #218789)

Used 'sizeof("add_")-1' since "add_" is const char [5].

233 ↗

(On Diff #218789)

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.

249 ↗

(On Diff #218789)

Good point. Let's avoid those false positives.

[hokein]

clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp

227 ↗

(On Diff #218789)

how about?

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

233 ↗

(On Diff #218789)

I wasn't aware of this corner case, could you add a comment describing the heuristic we use here (around the matcher)?

15 ↗

(On Diff #219180)

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)?

192 ↗

(On Diff #219180)

nit: const CXXMemberCallExpr *AppendCall =VectorAppendCall? VectorAppendCall:ProtoAddFieldCall;

195 ↗

(On Diff #219180)

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".

263 ↗

(On Diff #219180)

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?

[congliu]

Addressed Haojian's comments.

[hokein]

thanks, looks good.

clang-tools-extra/test/clang-tidy/performance-inefficient-vector-operation.cpp

315 ↗

(On Diff #219576)

looks like we miss (), the same below.

[congliu]

Addressed Haojian's comment.

[hokein]

feel free to commit the patch.

[congliu]

Could someone commit this for me? I failed to commit this change...

[hokein]

In D67135#1669836, @congliu wrote:

Could someone commit this for me? I failed to commit this change...

Committed in rL371963, just curious why you failed to commit? if you are using llvm git monorepo, ./llvm/utils/git-svn/git-llvm push should work.

clang-tools-extra/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst

 Finds possible inefficient ``std::vector`` operations (e.g. ``push_back``,
 ``emplace_back``) that may cause unnecessary memory reallocations.
 
-When EnableProto, also finds calls that add element to protobuf repeated field
-in a loop without calling Reserve() before the loop. Calling Reserve() first
-can avoid unnecessary memory reallocations.
+It can also find calls that add element to protobuf repeated field in a loop
+without calling Reserve() before the loop. Calling Reserve() first can avoid
+unnecessary memory reallocations.
 
 Currently, the check only detects following kinds of loops with a single
 statement body:

 
    Semicolon-separated list of names of vector-like classes. By default only
    ``::std::vector`` is considered.
+
+.. option:: EnableProto
+   When non-zero, the check will also warn on inefficient operations for proto
+   repeated fields. Otherwise, the check only warns on inefficient vector
+   operations. Default is `0`.