[PATCH] D131939: [clang-tidy] Add performance-expensive-flat-container-operation check

[Nicolas van Kempen via Phabricator via cfe-commits]

nicovank added a comment.

Thank you for the feedback!

In D131939#3822766 <https://reviews.llvm.org/D131939#3822766>, @njames93 wrote:

> I have a feeling the default should be to only warm in loops otherwise this could get noisy.

Agreed, the less noisy version was already the default, renaming it is a good idea to avoid confusions.



================
Comment at: clang-tools-extra/clang-tidy/performance/ExpensiveFlatContainerOperationCheck.cpp:70
+    const auto IsWithinLoop = cxxMemberCallExpr(
+        hasAncestor(stmt(anyOf(forStmt(), whileStmt(), doStmt())).bind("loop")),
+        // An easy false positive case: the variable is declared in the loop.
----------------
njames93 wrote:
> Rather than checking for an ancestor. Use the mapAnyOf matcher to check for a descendant in a loops body, this would remove the need for the false positive check below.
> Also you aren't checking a ranged for.
Thank you for catching the missing `cxxForRangeStmt`. Didn't know of `mapAnyOf`.

The idea is to not warn in cases such as below, where the declaration is in the same loop block as the insert/erase operation. Here, relatively, the insert is not in the loop. Of course, it's not possible to check the lifetime of the object in every case to make sure it's constrained to the loop, but from what I've seen this is the main false-positive case.

```
while(...) {
    flat_set<...> s;
    s.insert(...);
}
```

I don't think `mapAnyOf` makes expressing this any simpler (still need to check that the declaration is not within the same loop) so I kept the current version. If you really prefer `mapAnyOf` I can look into it again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131939