Page MenuHomePhabricator

[clang-tidy] Add performance-expensive-flat-container-operation check
Needs ReviewPublic

Authored by nicovank on Aug 15 2022, 8:47 PM.

Details

Summary

This check has been enabled internally at Facebook for a few months now (with OnlyWarnInLoops disabled), where folly::sorted_vector_map is pretty widely used.

I saw std::flat_(map|set) and std::flat_multi(map|set) will appear in C++23, at which point this check can be updated to include these.

folly::heap_vector_(map|set) is another example of such containers, though still relatively new so not included here yet.

Diff Detail

Event Timeline

nicovank created this revision.Aug 15 2022, 8:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2022, 8:47 PM
nicovank edited the summary of this revision. (Show Details)Aug 15 2022, 8:52 PM
nicovank edited the summary of this revision. (Show Details)
nicovank updated this revision to Diff 452886.Aug 15 2022, 9:05 PM
nicovank updated this revision to Diff 452894.Aug 15 2022, 10:44 PM

Rename second test file

nicovank updated this revision to Diff 452896.Aug 15 2022, 10:58 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/performance/ExpensiveFlatContainerOperationCheck.h
32

Please add isLanguageVersionSupported.

clang-tools-extra/docs/clang-tidy/checks/performance/expensive-flat-container-operation.rst
7

Please synchronize first statement with Release Notes.

Eugene.Zelenko added a project: Restricted Project.
nicovank updated this revision to Diff 453026.Aug 16 2022, 8:37 AM
nicovank marked an inline comment as done.

Add isLanguageVersionSupported.

nicovank published this revision for review.Aug 16 2022, 8:40 AM
nicovank marked an inline comment as done.

Ping.

I feel like Phabricator is not sending notifications for this patch like it usually does (I'm not getting any emails).
I'll create a new, identical patch tomorrow if there's still no activity.

I got notification.

I got notification.

Thank you. I got an email for this message but not my own updates, which usually also CC me 🤷.
Thanks!

Mid-CppCon ping.

I have a feeling the default should be to only warm in loops otherwise this could get noisy. Though setting it as the default you'd likely want to change the name to something along the lines of WarmOutsideLoops.

clang-tools-extra/clang-tidy/performance/ExpensiveFlatContainerOperationCheck.cpp
71

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.

nicovank updated this revision to Diff 464362.Fri, Sep 30, 12:25 PM

Rename OnlyWarnInLoops to WarnOutsideLoops, cover missed cxxForRangeStmt

Thank you for the feedback!

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.

clang-tools-extra/clang-tidy/performance/ExpensiveFlatContainerOperationCheck.cpp
71

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.