-
Notifications
You must be signed in to change notification settings - Fork 11.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[libcxx][test] Do not assume array::iterator is a pointer #100603
[libcxx][test] Do not assume array::iterator is a pointer #100603
Conversation
In the tests I added for `ranges::find_last{_if{_not}}`, I accidentally introduced an assumption that `same_as<array<T, 0>::iterator, T*>`; this is a faulty assumption on MSVC-STL.
@llvm/pr-subscribers-libcxx Author: nicole mazzuca (strega-nil) ChangesIn the tests I added for Full diff: https://github.com/llvm/llvm-project/pull/100603.diff 3 Files Affected:
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last.pass.cpp
index 2a2b12fb2c288..036631b19f48a 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last.pass.cpp
@@ -61,7 +61,8 @@ template <class It, class Sent = It>
constexpr void test_iterators() {
using ValueT = std::iter_value_t<It>;
auto make_range = [](auto& a) {
- return std::ranges::subrange(It(std::ranges::begin(a)), Sent(It(std::ranges::end(a))));
+ return std::ranges::subrange(
+ It(std::to_address(std::ranges::begin(a))), Sent(It(std::to_address(std::ranges::end(a)))));
};
{ // simple test
{
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if.pass.cpp
index a15f81bd4e481..427ef3539947f 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if.pass.cpp
@@ -59,7 +59,8 @@ static_assert(!HasFindLastIfR<ForwardRangeNotSentinelEqualityComparableWith>);
template <class It, class Sent>
constexpr auto make_range(auto& a) {
- return std::ranges::subrange(It(std::ranges::begin(a)), Sent(It(std::ranges::end(a))));
+ return std::ranges::subrange(
+ It(std::to_address(std::ranges::begin(a))), Sent(It(std::to_address(std::ranges::end(a)))));
}
template <template <class> class IteratorT, template <class> class SentinelT>
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if_not.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if_not.pass.cpp
index bb0e411acf0fa..f8357f9eecb93 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if_not.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if_not.pass.cpp
@@ -59,7 +59,8 @@ static_assert(!HasFindLastIfR<ForwardRangeNotSentinelEqualityComparableWith>);
template <class It, class Sent>
constexpr auto make_range(auto& a) {
- return std::ranges::subrange(It(std::ranges::begin(a)), Sent(It(std::ranges::end(a))));
+ return std::ranges::subrange(
+ It(std::to_address(std::ranges::begin(a))), Sent(It(std::to_address(std::ranges::end(a)))));
}
template <template <class> class IteratorT, template <class> class SentinelT>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM once CI is green, but I am surprised that this wasn't caught by our unstable-ABI job. That job should define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY
and _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW
, which use __wrap_iter
in array
and string_view
and should catch those issues.
@strega-nil There are still errors with MSVC's STL. Full logs: clang_errors.txt Here's my analysis of the first error, which is:
It appears that |
It looks like defining |
additionally, actually fix the rest of the tests now that I can actually test them.
@ldionne would you mind taking a look again? I've now actually changed product code (in |
The tests are now passing for MSVC's STL (completely with Clang; with MSVC modulo a compiler bug in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general it looks good, but I'd like to understand the constexpr
changes for the contiguous_iterator
✅ With the latest revision this PR passed the C/C++ code formatter. |
8857422
to
135531e
Compare
@strega-nil Off-topic question: are the changes to array and contigious_iterator supposed to fix this type of an error:
|
No, these would fix |
`array<T, 0>::iterator` was always a pointer even when `_LIBCXX_ABI_USE_WRAP_ITER_IN_STD_ARRAY` was defined. This patch fixes that minor bug. Discovered as part of [llvm#100603][]. [llvm#100603]: llvm#100603
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one request!
@@ -183,8 +183,17 @@ struct NonConstComparable { | |||
friend constexpr bool operator==(NonConstComparable&, const NonConstComparable&) { return true; } | |||
}; | |||
|
|||
// note: this should really use `std::const_iterator` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you open an issue and change this to a TODO instead please? AIUI we're only not doing that because const_iterator
isn't around yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll open an issue after merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I meant using the issue in the todo as TODO(<#issue-number>): <reason>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I meant using the issue in the todo as
TODO(<#issue-number>): <reason>
.
This was a part of my conditional LGTM. I will make it clearer in future reviews.
Merging as the only failure was agent lost, and the previous commit (before a comment-only change) did pass. |
`array<T, 0>::iterator` was always a pointer even when `_LIBCXX_ABI_USE_WRAP_ITER_IN_STD_ARRAY` was defined. This patch fixes that minor bug. Discovered as part of [llvm#100603][]. Drive-by: switch from `typedef` to `using` in `<array>` [llvm#100603]: llvm#100603
In the tests I added for `ranges::find_last{_if{_not}}`, I accidentally introduced an assumption that `same_as<array<T, 0>::iterator, T*>`; this is a faulty assumption on MSVC-STL. Fixes llvm#100498.
`array<T, 0>::iterator` was always a pointer even when `_LIBCXX_ABI_USE_WRAP_ITER_IN_STD_ARRAY` was defined. This patch fixes that minor bug. Discovered as part of [llvm#100603][]. Drive-by: switch from `typedef` to `using` in `<array>` [llvm#100603]: llvm#100603
In the tests I added for
ranges::find_last{_if{_not}}
, I accidentally introduced an assumption thatsame_as<array<T, 0>::iterator, T*>
; this is a faulty assumption on MSVC-STL.Fixes #100498.
cc @StephanTLavavej, can you test this PR with the MSVC-STL?