[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)

[Balazs Benics via cfe-commits]

================
@@ -0,0 +1,147 @@
+.. title:: clang-tidy - performance-redundant-lookup
+
+performance-redundant-lookup
+============================
+
+This check warns about potential redundant container lookup operations within
+a function.
+
+Examples
+--------
+
+.. code-block:: c++
+
+    if (map.count(key) && map[key] < threshold) {
+      // do stuff
+    }
+
+In this example, we would check if the key is present in the map,
+and then do a second lookup to actually load the value.
+We could refactor the code into this, to use a single lookup:
+
+.. code-block:: c++
+
+    if (auto it = map.find(key); it != map.end() && it->second < threshold) {
+      // do stuff
+    }
+
+In this example, we do three lookups while calculating, caching and then
+using the result of some expensive computation:
+
+.. code-block:: c++
+
+    if (!cache.contains(key)) {
+        cache[key] = computeExpensiveValue();
+    }
+    use(cache[key]);
+
+We could refactor this code using ``try_emplace`` to fill up the cache entry
+if wasn't present, and just use it if we already computed the value.
+This way we would only have a single unavoidable lookup:
+
+.. code-block:: c++
+
+    auto [cacheSlot, inserted] cache.try_emplace(key);
+    if (inserted) {
+        cacheSlot->second = computeExpensiveValue();
+    }
+    use(cacheSlot->second);
+
+
+What is a "lookup"?
+-------------------
+
+All container operations that walk the internal structure of the container
+should be considered as "lookups".
+This means that checking if an element is present or inserting an element
+is also considered as a "lookup".
+
+For example, ``contains``, ``count`` but even the ``operator[]``
+should be considered as "lookups".
+
+Technically ``insert``, ``emplace`` or ``try_emplace`` are also lookups,
+even if due to limitations, they are not recognized as such.
+
+Lookups inside macros are ignored, thus not considered as "lookups".
+For example:
+
+.. code-block:: c++
+
+    assert(map.count(key) == 0); // Not considered as a "lookup".
+
+Options
+-------
+
+.. option:: ContainerNameRegex
+
+   The regular expression matching the type of the container objects.
+   This is matched in a case insensitive manner.
+   Default is ``set|map``.
----------------
steakhal wrote:

I share your concern. However, there is more to consider here.

I think, this check should work with user-defined containers out of the box, to have a better off the shelf experience.

If they find FPs, they can override this regex, and that's it.

I think the chances as slim to have container-like APIs like I'm matching here. For example, exatcly single argument "insert".

I could restrict the matches by requiring more things. For example, `key_type` type alias defined in the container, or something like that.
However, for doing that, I'd first ask if this misclassification really happens in practice to justify the complexity.

My experience with the FPs is that there are sharp edges with the check, but this one is not one of those.

https://github.com/llvm/llvm-project/pull/125420