Page MenuHomePhabricator

[libc++] Refactor __less
Needs RevisionPublic

Authored by philnik on Fri, Mar 3, 4:25 PM.

Details

Reviewers
ldionne
Mordante
EricWF
Group Reviewers
Restricted Project
Summary

This simplifies the usage of __less by making the class not depend on the types compared, but instead the operator(). We can't remove the template completely because we explicitly instantiate std::__sort with __less<T>.

Diff Detail

Unit TestsFailed

TimeTest
1,210 mslibcxx CI C++03 > llvm-libc++-shared-cfg-in.libcxx/algorithms::robust_against_cpp20_hostile_iterators.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/libcxx/test/libcxx/algorithms/robust_against_cpp20_hostile_iterators.compile.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++03 -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -Wno-ambiguous-reversed-operator -fsyntax-only
1,730 mslibcxx CI C++03 > llvm-libc++-shared-cfg-in.std/algorithms::robust_re_difference_type.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/libcxx/test/std/algorithms/robust_re_difference_type.compile.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++03 -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -fsyntax-only
1,170 mslibcxx CI C++03 > llvm-libc++-shared-cfg-in.std/algorithms/alg_sorting/alg_binary_search/binary_search::binary_search.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/libcxx/test/std/algorithms/alg.sorting/alg.binary.search/binary.search/binary_search.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++03 -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/test/std/algorithms/alg.sorting/alg.binary.search/binary.search/Output/binary_search.pass.cpp.dir/t.tmp.exe
1,220 mslibcxx CI C++03 > llvm-libc++-shared-cfg-in.std/algorithms/alg_sorting/alg_binary_search/equal_range::equal_range.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/libcxx/test/std/algorithms/alg.sorting/alg.binary.search/equal.range/equal_range.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++03 -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/test/std/algorithms/alg.sorting/alg.binary.search/equal.range/Output/equal_range.pass.cpp.dir/t.tmp.exe
1,450 mslibcxx CI C++03 > llvm-libc++-shared-cfg-in.std/algorithms/alg_sorting/alg_binary_search/lower_bound::lower_bound.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/libcxx/test/std/algorithms/alg.sorting/alg.binary.search/lower.bound/lower_bound.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++03 -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/ff401cdcac7e-1/llvm-project/libcxx-ci/build/generic-cxx03/test/std/algorithms/alg.sorting/alg.binary.search/lower.bound/Output/lower_bound.pass.cpp.dir/t.tmp.exe
View Full Test Results (127 Failed)

Event Timeline

philnik created this revision.Fri, Mar 3, 4:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Mar 3, 4:25 PM
Herald added a subscriber: mgrang. · View Herald Transcript
philnik requested review of this revision.Fri, Mar 3, 4:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Mar 3, 4:25 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
EricWF requested changes to this revision.Tue, Mar 7, 1:32 PM
EricWF added a subscriber: EricWF.

This cleanup seems incomplete. Why keep __less as a template at all given it's never actually used as one?

Also does the iterator's ::value_type always have to exactly match the type returned from dereferencing it?

libcxx/include/__algorithm/comp.h
37

This doesn't at all seem correct to me.

This takes away an explicit conversion

libcxx/include/__algorithm/includes.h
64

Is this correct? What if the iterator returns a type convertible to value_type?

This revision now requires changes to proceed.Tue, Mar 7, 1:32 PM
EricWF added inline comments.Tue, Mar 7, 1:32 PM
libcxx/include/__algorithm/comp.h
37

*The implicit conversion.

This cleanup seems incomplete. Why keep __less as a template at all given it's never actually used as one?

If you block other reviews on there not being a description, at least read it.

libcxx/include/__algorithm/includes.h
64

AFAICT the standard it either very silent on this, or says one should use less{} as the comparator, which does what __less<>{} does after this change. So this change is either a conforming change, or arguably a bugfix. Although funnily enough std::less{} isn't actually well-formed, and it doesn't specifically use ranges::less{} for the classic algorithms.

philnik updated this revision to Diff 503404.Wed, Mar 8, 9:15 AM

Try to fix CI

ldionne requested changes to this revision.Thu, Mar 9, 9:22 AM
ldionne added inline comments.
libcxx/include/__algorithm/comp.h
32

So there's actually a behavior change in this patch (the removal of implicit conversions noticed by @EricWF). However, I think it is desirable -- right now I don't think we are conforming in C++20. For example, take lower_bound: http://eel.is/c++draft/lower.bound#1. It specifically says Let comp be less{} and proj be identity{} for overloads with no parameters by those names., which implies that we should be using the transparent comparator, but we are not (before this patch).

I think this mandates a change everywhere along with tests. We should also confirm what's the expected behavior in prior Standards and make sure we conform in those as well.

33

Please add a comment explaining why it's empty.

This revision now requires changes to proceed.Thu, Mar 9, 9:22 AM