[PATCH] D146061: [ADT] Make llvm::is_contained call member `contains` when available

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.

In D146061#4194866 <https://reviews.llvm.org/D146061#4194866>, @kuhar wrote:

> In D146061#4194853 <https://reviews.llvm.org/D146061#4194853>, @nikic wrote:
>
>> In D146061#4194752 <https://reviews.llvm.org/D146061#4194752>, @kuhar wrote:
>>
>>> Sure, I agree with that. If I have map with a known type like `DenseMap` I'd rather call `.contains(x)` directly too. But this is not possible with C++17 containers `std::`, so I'd argue that until then `is_contained(container, x)` is more readable than something like `container.find(x) != container.end()`.
>>
>> I don't think we should promote usage of is_contained() for anything but the linear scan case. I don't want to be left guessing whether this is_contained() is a linear scan, or just a slightly different way to write container.find().
>
> I can see this being prefered from the perspective of an experience llvm developer, but on the other hand I saw a lot of code that ended up using `is_contained` and `llvm::find` with map/set types. I found fixing those inefficiencies and re-explaining this to folks needlessly time consuming.  I think defaulting to the more efficient implementation of containment check is the way to go to avoid those non-obvious performance issues, and the member functions we would delegate to are not surprising at all.

Agreed with the analysis.



================
Comment at: llvm/unittests/ADT/STLExtrasTest.cpp:1039
+      return ContainsResult;
+    }
+    bool ContainsResult = false;
----------------
Add a `find` to show that `contains` is preferred.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146061