Skip to content
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

Revert "[STLExtras] Remove incorrect hack to make indexed_accessor_range operator== compatible with C++20" #72265

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

usx95
Copy link
Contributor

@usx95 usx95 commented Nov 14, 2023

Reverts #72220

This breaks C++20 build bot. Need to see if upgrading to clang-17 in the build bot would solve the issue.

…nge operator== compatible with C++20 (#72220)"

This reverts commit 2be3fca.
@usx95 usx95 merged commit 94d6699 into main Nov 14, 2023
3 of 4 checks passed
@usx95 usx95 deleted the revert-72220-remove-friend-op-eq branch November 14, 2023 15:07
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 14, 2023

@llvm/pr-subscribers-llvm-adt

Author: Utkarsh Saxena (usx95)

Changes

Reverts llvm/llvm-project#72220

This breaks C++20 build bot. Need to see if upgrading to clang-17 in the build bot would solve the issue.


Full diff: https://github.com/llvm/llvm-project/pull/72265.diff

1 Files Affected:

  • (modified) llvm/include/llvm/ADT/STLExtras.h (+9-5)
diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index e42b0e8d1b27b71..18bc4d108b156bf 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -1291,11 +1291,15 @@ class indexed_accessor_range_base {
   }
 
   /// Compare this range with another.
-  template <typename OtherT> bool operator==(const OtherT &rhs) const {
-    return std::equal(begin(), end(), rhs.begin(), rhs.end());
-  }
-  template <typename OtherT> bool operator!=(const OtherT &rhs) const {
-    return !(*this == rhs);
+  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);
   }
 
   /// Return the size of this range.

@dwblaikie
Copy link
Collaborator

Probably the right thing here would've been to commit the revert without sending a pull request.

Specifically, for now, a pull request is a signal that this patch needs review - so committing a sent pull request without approval is an indication of process failure/that code that needed review was committed without review. So, please don't do that (either commit directly if it doesn't need precommit review, or post a pull request and wait for review if it needs precommit review)

@usx95
Copy link
Contributor Author

usx95 commented Nov 15, 2023

Thanks for the clarification. I was not aware of that practice.
FWIW, Github offers to revert a PR with 2-clicks which is extremely convenient (1. create revert PR, 2. merge). If we are not to follow this for LLVM (because PR implies a code review), we should mention not to use this method in the docs

@dwblaikie
Copy link
Collaborator

Ah, good to know - I'll see about getting the docs updated.

zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
…nge operator== compatible with C++20" (llvm#72265)

Reverts llvm#72220

This breaks C++20 build bot. Need to see if upgrading to clang-17 in the
build bot would solve the issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants