Page MenuHomePhabricator

is_contained() convenience function
ClosedPublic

Authored by Alexander_Droste on Jan 11 2016, 2:08 AM.

Details

Summary

This patch adds a convenience is_contained() function
which checks if an element exists in a container.

This patch was originally part of http://reviews.llvm.org/D12761.

Diff Detail

Repository
rL LLVM

Event Timeline

Alexander_Droste retitled this revision from to contains() convenience function.
Alexander_Droste updated this object.
Alexander_Droste updated this object.
Alexander_Droste added a reviewer: dblaikie.
  • changed name to is_contained() because contains() caused name conflicts
  • adapted the style of other functions in STLExtras.h
  • @david: I hope its ok that I added you as a reviewer. Googling for 'stlextras llvm reviewer' you

were the first author that appeared.

dblaikie edited edge metadata.Jan 11 2016, 8:43 AM
dblaikie added a subscriber: dblaikie.

Benjamin and I were just discussing a similar piece of code to this in a
review

I think we were settling on something like "any_of(R, is_equal<>(E))" but
I'm OK adding something like "contains" or "is_contained" (contains seems
like the right name, but might be too often used elsewhere to be
convenient?)

Benjamin - any thoughts/preferences here?

Alexander_Droste retitled this revision from contains() convenience function to is_contained() convenience function.
Alexander_Droste edited edge metadata.
  • changed implementation to std::equal_to<> style

I think in the last review I/we were just suggesting to use any_of +
equal_to directly & skip adding this extra construct entirely.

In any case, std::equal_to is a C++14 feature, so we probably can't use it
in LLVM & the suggestion/discussion was about whether we could/should just
implement one in LLVM (llvm::equal_to). But you also make a good point
about equal_to needing a parameter/binding, blah blah - Ben: thoughts on
that? Still nice if we have to use bind too? Could/should we make another
function object to handle that more easily (the general case of a stateful
unary equal_to, I mean)?

@Benjamin : I think your expertise is needed. :)

I personally dislike bind. I thought there was a a way to use std::equal_to without manually binding, but apparently there isn't, so adding is_contained is fine with me. Preferably with std::find as it avoids a confusing bind.

  • reverted implementation, in order to be based on find instead of any_of/std::equal_to<E>()
  • removed whitespace:

bool is_contained(R && Range,..
->
bool is_contained(R &&Range,..

dblaikie accepted this revision.Feb 3 2016, 2:21 PM
dblaikie edited edge metadata.
This revision is now accepted and ready to land.Feb 3 2016, 2:21 PM

Hi,

as it seems, this patch has not been committed yet.
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/ADT/STLExtras.h
I have no commit access. Can someone commit this for me?

This revision was automatically updated to reflect the committed changes.