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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146061