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

Sema: @memcpy changes #22631

Merged
merged 1 commit into from
Jan 29, 2025
Merged

Sema: @memcpy changes #22631

merged 1 commit into from
Jan 29, 2025

Conversation

mlugg
Copy link
Member

@mlugg mlugg commented Jan 28, 2025

  • The langspec definition of @memcpy has been changed so that the source and destination element types must be in-memory coercible, allowing all such calls to be raw copying operations, not actually applying any coercions.
  • Implement aliasing check for comptime @memcpy; a compile error will now be emitted if the arguments alias.
  • Implement more efficient comptime @memcpy by loading and storing a whole array at once, similar to how @memset is implemented.

This is a breaking change because while the old coercion behavior triggered a Sema TODO at runtime, it did actually work at comptime.

@mlugg mlugg added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. release notes This PR should be mentioned in the release notes. labels Jan 28, 2025
@xdBronch
Copy link
Contributor

closes #21748

/// * for `[]T`, returns `T`
/// * for `[*]T`, returns `T`
/// * for `[*c]T`, returns `T`
pub fn indexablePtrElem(ty: Type, zcu: *const Zcu) Type {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not just elemType2? (surely we don't have too many helpers)

Copy link
Member Author

Choose a reason for hiding this comment

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

elemType2 does do this, but I try to avoid using that helper because it's far too general (it does several unrelated things). I'll hopefully kill it soon :P

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, yeah, I suspect that most (if not all) of the usages of elemType2 really just want indexablePtrElem. But that function also implements these extra cases:

  • ?T -> std.meta.Child(T) (its doc comment is a lie; e.g. it does ?*[n]T -> [n]T)
  • anyframe->T -> T (of course, that's unused right now)
  • [n]T -> T
  • @Vector(n, T) -> T

src/Sema.zig Show resolved Hide resolved
@alexrp

This comment was marked as off-topic.

* The langspec definition of `@memcpy` has been changed so that the
  source and destination element types must be in-memory coercible,
  allowing all such calls to be raw copying operations, not actually
  applying any coercions.
* Implement aliasing check for comptime `@memcpy`; a compile error will
  now be emitted if the arguments alias.
* Implement more efficient comptime `@memcpy` by loading and storing a
  whole array at once, similar to how `@memset` is implemented.
Copy link
Contributor

@dweiller dweiller left a comment

Choose a reason for hiding this comment

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

The langref should be updated as well. It contains language about allowing different ABI sizes

@mlugg
Copy link
Member Author

mlugg commented Jan 29, 2025

The langref should be updated as well. It contains language about allowing different ABI sizes

Oh, you're right. I'm going to merge this as-is, but will put up another PR straight after with the langref changes.

@mlugg mlugg merged commit 71d1610 into ziglang:master Jan 29, 2025
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. release notes This PR should be mentioned in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants