Skip to content

Fix Box(Pointer).box to not allocate pointer storage on the heap#15562

Merged
straight-shoota merged 1 commit intocrystal-lang:masterfrom
ysbaddaden:fix/dont-allocate-to-box-pointer
Mar 21, 2025
Merged

Fix Box(Pointer).box to not allocate pointer storage on the heap#15562
straight-shoota merged 1 commit intocrystal-lang:masterfrom
ysbaddaden:fix/dont-allocate-to-box-pointer

Conversation

@ysbaddaden
Copy link
Collaborator

@ysbaddaden ysbaddaden commented Mar 17, 2025

While discussing to use Box(T) in #15395 I noticed that Box(Pointer).box(ptr) allocates into the GC heap to store the pointer address into.

That felt like an overlook since we don't need to allocate to box a Reference or Nil, so it should be fine to box a raw pointer without allocating an intermediary pointer. Am I missing something?

I only allowed T < Pointer in addition to the Reference and nilable Reference cases. I'm not sure a Pointer appearing in an union along Nil or Reference would behave correctly —is it passing the {type, value} union by value?

@straight-shoota straight-shoota added this to the 1.16.0 milestone Mar 17, 2025
@straight-shoota straight-shoota changed the title Fix: Box(T).box allocates when T < Pointer Fix passing a pointer to Box(T).box without allocation Mar 19, 2025
@straight-shoota straight-shoota changed the title Fix passing a pointer to Box(T).box without allocation Fix Box(Pointer).box to not allocate pointer storage on the heap Mar 19, 2025
@straight-shoota
Copy link
Member

Pointer | Nil and Pointer | Reference are mixed union types, so they cannot be represented by just a pointer. We need to allocate on the heap. They are not very common types, thus it doesn't matter much.
We might consider adding specs for these types though, to make sure the behaviour is correct (can be in this PR or a followup).

@straight-shoota straight-shoota merged commit 63121de into crystal-lang:master Mar 21, 2025
33 of 34 checks passed
@HertzDevil
Copy link
Contributor

HertzDevil commented Mar 21, 2025

If this is allowed, then perhaps values smaller than a pointer also don't need an allocation either? Primitive numbers for example:

struct Box(T)
  def self.box(object : T) : Void*
    {% if T <= Int::Primitive && sizeof(T) <= sizeof(Void*) %}
      Pointer(Void).new(object.to_u64!)
    {% end %}
  end

  def self.unbox(pointer : Void*) : T
    {% if T <= Int::Primitive && sizeof(T) <= sizeof(Void*) %}
      T.new!(pointer.address)
    {% end %}
  end
end

@ysbaddaden ysbaddaden deleted the fix/dont-allocate-to-box-pointer branch March 21, 2025 12:59
@ysbaddaden
Copy link
Collaborator Author

Since the pointer returned by .box can only safely be used with .unbox(ptr) then I guess it would be fine 🤔

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