[PATCH] D46317: [clang-tidy] New check bugprone-map-subscript-operator-lookup

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D46317#2027330 <https://reviews.llvm.org/D46317#2027330>, @khuttun wrote:

> In D46317#2027071 <https://reviews.llvm.org/D46317#2027071>, @aaron.ballman wrote:
>
> > In D46317#2023406 <https://reviews.llvm.org/D46317#2023406>, @khuttun wrote:
> >
> > > Any comments on this? Is this checker something that could be part of clang-tidy?
> >
> >
> > Thank you for posting some of the diagnostics found by the check, that was really helpful information! I spot-checked ~10 of the issues it reported and all of them were false positives. Were you able to find any true positives from that list? I think 1200 reports without any true positives indicates that the check may be too chatty to include (it may also suggest that `bugprone` is the wrong place for the check).
>
>
> It's difficult to spot actual functionality bugs without knowing the code better, but there's plenty of unnecessary double-lookups (count()/find() + operator[]) reported, for example:


Definitely agreed that it's labor-intensive. :-)

> clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp:75:30
>  clang-tools-extra/clang-tidy/bugprone/ForwardDeclarationNamespaceCheck.cpp:157:33
>  clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:106:29

I hadn't spot checked those, but did see at least one similar case with what I checked. However, I consider those false positives because none of these are cases where the code gives wrong results. It is possible they are performance concerns (though I'd be curious whether these patterns optimize well or not), so if the check was specific to finding that scenario, that would be pretty useful (though it likely requires codeflow analysis).


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

https://reviews.llvm.org/D46317