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

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun marked an inline comment as done.
khuttun added a comment.

In D46317#1988261 <https://reviews.llvm.org/D46317#1988261>, @lebedev.ri wrote:

> IMHO we should proceed with this patch.
>  There's been several patches recently that seem to not care about false-positive rate,
>  and in this case the cost of true-positive can be humongous, as i/we've been finding
>  lots of bugs exactly like this in openmp optimization pass, attributor framework :/


When developing this checker I tested it on LLVM code base, and there was hundreds of warnings. Looking at my notes from then, the ones that I analyzed fell in to couple of different categories:

- The program logic is such that it's known that the key is found in the map (in some of these cases `count()` is asserted before the lookup)
- The code does `find()` vs.` end()` comparison before the lookup

>From these the at least the first category could be considered false positives.

I also think that this checker could bring value, though. For example when enabled on a new project from the start. I should have time to finish it if reviewers feel that we should reconsider it. Also, go ahead and share if you have any ideas on how to make the checker more accurate.



================
Comment at: test/clang-tidy/bugprone-map-subscript-operator-lookup.cpp:60
+}
+
+void noWarning() {
----------------
mgehre wrote:
> We often have code like
> ```
> auto &V = Map[Idx];
> if (!V) {
>   V = init();
> }
> use(V);
> ```
> Would that be flagged by this check? Could you add a test for it (possibly with `FIXME`)?
This would not be flagged, as the reference is non-const


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D46317