This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] fix `std::search_n` incorrect `static_assert`
ClosedPublic

Authored by huixie90 on Jul 19 2022, 2:00 PM.

Details

Summary

[libc++][ranges] fix std::search_n incorrect static_assert
see more detail in https://reviews.llvm.org/D124079?#3661721

Diff Detail

Event Timeline

huixie90 created this revision.Jul 19 2022, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 2:00 PM
huixie90 requested review of this revision.Jul 19 2022, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 2:00 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
jloser added a subscriber: jloser.Jul 19 2022, 2:14 PM
jloser added inline comments.
libcxx/test/std/algorithms/alg.nonmodifying/alg.search/search_n_pred.pass.cpp
181–189

Nit: can we give a better name to this function?

huixie90 added inline comments.Jul 19 2022, 2:30 PM
libcxx/test/std/algorithms/alg.nonmodifying/alg.search/search_n_pred.pass.cpp
181–189

Any suggestions? I agree it is a terrible name. It is my failed attempt to give it a reasonable name

jloser added inline comments.Jul 19 2022, 2:36 PM
libcxx/test/std/algorithms/alg.nonmodifying/alg.search/search_n_pred.pass.cpp
181–189

Maybe test_binary_predicate_callable? Note the camel case instead to match that of test_constexpr for example.

ldionne accepted this revision.Jul 19 2022, 2:40 PM
ldionne added a subscriber: ldionne.

Should we add a similar test in std::search and others that have a similar pattern?

libcxx/test/std/algorithms/alg.nonmodifying/alg.search/search_n_pred.pass.cpp
181–189

Suggestion -- don't give it a name. Define it in main like this instead:

// test bug reported in https://reviews.llvm.org/D124079?#3661721
{
  A a[]       = {A(1, 2), A(2, 3), A(2, 4)};
  int value   = 2;
  auto result = std::search_n(a, a + 3, 1, value, Pred());
  assert(result == a + 1);
}
This revision is now accepted and ready to land.Jul 19 2022, 2:40 PM

Should we add a similar test in std::search and others that have a similar pattern?

std::search takes two ranges so it is slightly different. But yes it is worthwhile checking other algorithms which take a range and a value to see if they work properly

libcxx/test/std/algorithms/alg.nonmodifying/alg.search/search_n_pred.pass.cpp
181–189

good suggestion. thanks

huixie90 updated this revision to Diff 445953.Jul 19 2022, 3:00 PM
huixie90 marked 2 inline comments as done.

address comments

This revision was automatically updated to reflect the committed changes.

Thanks a lot for the quick fix!!