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

Tue Mar 14 13:30:25 PDT 2023

dblaikie added a comment.

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

> In D146061#4193557 <https://reviews.llvm.org/D146061#4193557>, @nikic wrote:
>
>>> I also considered detecting member contains and triggering a
>>> static_assert instead, but decided against it because it's just as easy
>>> to do the right thing and call .contains.
>>
>> I think I would prefer this option, so we keep the ability to distinguish O(1) contains from O(n) is_contained at a glance.
>
> The way I was thinking about this is that usually the C++ standard specifies algorithms in terms of worst-case complexity and anything better is a bonus. In a completely generic context, as a user of `is_contained` my expectation would be that it's O(n) or better.
>
> This situation is a bit more complicated because C++20 is added member contains functions to (`unordered_`)`set`/`map`, so a static assertion would cause compilation failures with some compiler flags and not the other ones. IMO this is not a huge deal but probably annoying because I'd expect the Hyrum law to apply and API usages of is_contained with these containers to creep in over time. The current patch makes this API instability problem better , as there'll be no need to have ways to essentially do the same thing.

Yeah, that'd be unfortunate indeed.

> Another thing I intended to change was to make `llvm::find` call member `find` when present. This would make it and also `llvm::is_contained` O(1)/O(log n) even for types that do not expose `.contains` today. The alternative would be to disallow `llvm::find` over container with member find as well, but this doesn't seem like the best choice for me.

This one is trickier for me - `llvm::is_contained` doesn't have a nearby standard equivalent ,so we can choose its behavior more flexibly. But `std::find(iter, iter, value)` exists in the standard library and there's value in `llvm::find(range, value)` being pretty close to the same/identical to the standard version - which doesn't optimize to member-find when available.

I would be inclined not to include a member-find optimization in `llvm::find` for that reason.

I'm undecided about whether that tips in favor of not having member-contains in `llvm::is_contained`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146061