[STLExtras] Add out-of-line definition of friend operator== for C++20 #72348
Conversation
|
@llvm/pr-subscribers-llvm-adt Author: Utkarsh Saxena (usx95) ChangesThe last attempt at #72220 was reverted by 94d6699 because it breaks C++20 build in clang-17 and before. This is a workaround of #70210 and unblocks #72213 which rectifies rewriting template operator and thus introduces new breakages. Moving the function definition out of the class makes clang find a matching The final plan, when #70210 is fixed, is to move these back to inline definition or even convert to a member template operator. This should not be urgent and could even wait for a major clang release including #72213 Full diff: https://github.com/llvm/llvm-project/pull/72348.diff 1 Files Affected:
diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index 18bc4d108b156bf..380d4d461c4e8db 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -1291,16 +1291,19 @@ class indexed_accessor_range_base {
}
/// Compare this range with another.
- template <typename OtherT>
- friend bool operator==(const indexed_accessor_range_base &lhs,
- const OtherT &rhs) {
- return std::equal(lhs.begin(), lhs.end(), rhs.begin(), rhs.end());
- }
- template <typename OtherT>
- friend bool operator!=(const indexed_accessor_range_base &lhs,
- const OtherT &rhs) {
- return !(lhs == rhs);
- }
+ // FIXME: Make me a member function instead of friend when it works in C++20.
+ template <typename OtherT, typename DerivedT2, typename BaseT2, typename T2,
+ typename PointerT2, typename ReferenceT2>
+ friend bool
+ operator==(const indexed_accessor_range_base<DerivedT2, BaseT2, T2, PointerT2,
+ ReferenceT2> &lhs,
+ const OtherT &rhs);
+ template <typename OtherT, typename DerivedT2, typename BaseT2, typename T2,
+ typename PointerT2, typename ReferenceT2>
+ friend bool
+ operator!=(const indexed_accessor_range_base<DerivedT2, BaseT2, T2, PointerT2,
+ ReferenceT2> &lhs,
+ const OtherT &rhs);
/// Return the size of this range.
size_t size() const { return count; }
@@ -1364,6 +1367,23 @@ class indexed_accessor_range_base {
/// The size from the owning range.
ptrdiff_t count;
};
+
+// FIXME: Make me a member function instead of friend when it works in C++20.
+template <typename OtherT, typename DerivedT2, typename BaseT2, typename T2,
+ typename PointerT2, typename ReferenceT2>
+bool operator==(const indexed_accessor_range_base<DerivedT2, BaseT2, T2,
+ PointerT2, ReferenceT2> &lhs,
+ const OtherT &rhs) {
+ return std::equal(lhs.begin(), lhs.end(), rhs.begin(), rhs.end());
+}
+
+template <typename OtherT, typename DerivedT2, typename BaseT2, typename T2,
+ typename PointerT2, typename ReferenceT2>
+bool operator!=(const indexed_accessor_range_base<DerivedT2, BaseT2, T2,
+ PointerT2, ReferenceT2> &lhs,
+ const OtherT &rhs) {
+ return !(lhs == rhs);
+}
} // end namespace detail
/// This class provides an implementation of a range of
|
usx95
changed the title
|
This would need a cherry-pick for issue #68901 to get into 17.0.6. After that we should be able to submit this and upgrade C++20 bot to 17.0.6. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Update: I verified that this change works fine with clang-17.0.6 and fails with clang-17.0.5 in C++20. |
|
I am awaiting C++20 build bots to be updated to 17.0.6. (cc: @ilya-biryukov) FYI: @cor3ntin @AaronBallman with this change, clang 17.0.6 would be the minimum version to build LLVM in C++20 after this change. |
|
@usx95 stupid question, how does that affects llvm when built with GCC/MSVC? |
It should be announced in the release notes as well as the Announcements channel so people are aware. That said, I am still concerned with bumping the minimum version up that much. Clang 5.0 (Sep 2017) to Clang 17.0 (Nov 2023) is quite the jump. This might not be a bad thing to float via an RFC in Discourse before announcing it as our final decision just to make sure we get community buy-in. I don't think it will be contentious (there's not much to be done; when we want to switch to C++20 for the code base, we're going to need to make a significant leap anyway), but best to make sure this won't be disruptive to anyone in ways that we can address. |
|
Sent out an RFC. |
See llvm/llvm-project#72348 for some context on why we need this upgrade.
We need this upgrade to roll forward fixes to C++20 support in Clang itself. For more context, see llvm/llvm-project#72348 and https://discourse.llvm.org/t/rfc-clang-17-0-6-would-be-minimum-version-to-build-llvm-in-c-20/
|
Build bot has been updated, and the RFC has no pushback. I will land this now. |
…llvm#72348) The last attempt at llvm#72220 was reverted by llvm@94d6699 because it breaks C++20 build in clang-17 and before. This is a workaround of llvm#70210 and unblocks llvm#72213 which rectifies rewriting template operator and thus introduces new breakages. Moving the function definition out of the class makes clang find a matching `operator!=` for the `operator==`. This makes clang not rewrite the `operator==` with reversed args. Hence, the ambiguity is resolved. The final plan, when llvm#70210 is fixed, is to move these back to inline definition or even convert to a member template operator. This should not be urgent and could even wait for a major clang release including llvm#72213
The last attempt at #72220 was reverted by 94d6699 because it breaks C++20 build in clang-17 and before.
This is a workaround of #70210 and unblocks #72213 which rectifies rewriting template operator and thus introduces new breakages.
Moving the function definition out of the class makes clang find a matching
operator!=for theoperator==. This makes clang not rewrite theoperator==with reversed args. Hence, the ambiguity is resolved.The final plan, when #70210 is fixed, is to move these back to inline definition or even convert to a member template operator. This should not be urgent and could even wait for a major clang release including #72213