Skip to content

[ADT] Fix ArrayRef<T>::slice #113048

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

Merged
merged 1 commit into from
Oct 23, 2024
Merged

[ADT] Fix ArrayRef<T>::slice #113048

merged 1 commit into from
Oct 23, 2024

Conversation

FLZ101
Copy link
Contributor

@FLZ101 FLZ101 commented Oct 19, 2024

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Oct 19, 2024

@llvm/pr-subscribers-llvm-adt

Author: Franklin (FLZ101)

Changes

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

1 Files Affected:

  • (modified) llvm/include/llvm/ADT/ArrayRef.h (+4-1)
diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h
index ac40ec4a6b2404..69a08fd079043a 100644
--- a/llvm/include/llvm/ADT/ArrayRef.h
+++ b/llvm/include/llvm/ADT/ArrayRef.h
@@ -198,7 +198,10 @@ namespace llvm {
     }
 
     /// slice(n) - Chop off the first N elements of the array.
-    ArrayRef<T> slice(size_t N) const { return slice(N, size() - N); }
+    ArrayRef<T> slice(size_t N) const {
+      assert(N <= size() && "Invalid specifier");
+      return slice(N, size() - N);
+    }
 
     /// Drop the first \p N elements of the array.
     ArrayRef<T> drop_front(size_t N = 1) const {

Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the PR description and explain what the issue is?

Comment on lines 202 to 203
assert(N <= size() && "Invalid specifier");
return slice(N, size() - N);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, we could call into drop_front which does the same thing. Or make drop_front call slice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think drop_front is more idiomatic and we could probably mark the single-argument slice as deprecated and eventually remove it. Although that's all out of scope for this PR. cc: @kazutakahirata

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented slice(N) with drop_front(N).

@FLZ101
Copy link
Contributor Author

FLZ101 commented Oct 20, 2024

Could you update the PR description and explain what the issue is?

slice(N, size() - N) will never fail the assertion assert(N+M <= size() && "Invalid specifier") above, even N > size().

Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but please add the explanation from the comment to the commit description when merging.

Current implementation of `slice(N)` is buggy, since
`slice(N, size() - N)` will never fail the assertion
`assert(N+M <= size() && "Invalid specifier")` above, even
`N > size()`.
@FLZ101
Copy link
Contributor Author

FLZ101 commented Oct 21, 2024

Looks good but please add the explanation from the comment to the commit description when merging.

Done.

@FLZ101
Copy link
Contributor Author

FLZ101 commented Oct 22, 2024

@kuhar Could you merge this PR?

@kuhar kuhar merged commit deb22fa into llvm:main Oct 23, 2024
8 checks passed
@frobtech frobtech mentioned this pull request Oct 25, 2024
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