search

crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.36k stars 1.62k forks source link

Remove calls to `Pointer.new(Int)` #14683

Closed straight-shoota closed 3 months ago

straight-shoota commented 3 months ago

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 commented 3 months ago

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

straight-shoota commented 3 months ago

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:

https://github.com/crystal-lang/crystal/blob/897bd10f260dd540257348d0461b25d3fff97461/src/pointer.cr#L421-L423

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 commented 3 months ago

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 :thinking:

straight-shoota commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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.