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

Remove calls to Pointer.new(Int) #14683

Conversation

straight-shoota
Copy link
Member

This is a preparatory step to dropping the overload Pointer.new(Int) (#14669). It replaces all calls to this overloads with explicit casts to UInt64 at the call site, which ends up resolving to the main overload Pointer.new(UInt64).

@HertzDevil
Copy link
Contributor

Do you think this is fine for 32-bit targets or even AVR?

@straight-shoota
Copy link
Member Author

straight-shoota commented Jun 10, 2024

Yes, it's fine. This is not changing anything from the current status. So compatibility with smaller architectures is not really relevant to this PR. Everything that worked before this PR, should continue to work.

Pointer.new(Int) is implemented the same way:

crystal/src/pointer.cr

Lines 421 to 423 in 897bd10

def self.new(address : Int)
new address.to_u64!
end

This patch merely pulls out the cast to_u64! to the call size.

A portable implementation would be possible with a uintptr_t type (and appropriate cast methods).

@ysbaddaden
Copy link
Contributor

The one advantage is that now these pointer constants will be actual consts, and no more wrapped in a Crystal::Once to be defined at runtime.

Though I'm wondering if we couldn't introduce a Pointer::Size type (currently set to UInt64) to prepare for the introduction of a target specific pointer size 🤔

@straight-shoota
Copy link
Member Author

@ysbaddaden Yes, I think we should do that. But it's a separate change. We can easily come back to update these definitions then. And there'll be a lot more which need this treatment.

@ysbaddaden
Copy link
Contributor

Are you sure it's separate? Introducing a target specific Pointer::Size might be, but introducing a hardcoded Pointer::Size as UInt64 seems like it would fit just right in to me.

@straight-shoota
Copy link
Member Author

straight-shoota commented Jun 11, 2024

IMO introducing Pointer::Size even if as an alias to UInt64 is a separate change. Now we could pull that up and merge it before this PR. But I see little benefit. They can be merged in any order.

@straight-shoota straight-shoota added this to the 1.13.0 milestone Jun 17, 2024
@straight-shoota straight-shoota merged commit 1f3a4fa into crystal-lang:master Jun 18, 2024
61 checks passed
@straight-shoota straight-shoota deleted the refactor/pointer-new-int-calls branch June 18, 2024 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants