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

feat: don't manage blittable types #1407

Merged
merged 15 commits into from
Apr 4, 2022
Merged

Conversation

scolsen
Copy link
Contributor

@scolsen scolsen commented Apr 1, 2022

The blit interface, when implemented, indicates that a type is freely
copyable and does not require deletion. It is similar to Rust's Copy
trait and indicates that a type should have copy instead of "move"
semantics. This commit updates the memory management system to ignore
such types, since we do not need to worry about deleting them.

Thus, when using a blittable type, users can freely use a variable of
the type in more than one place.

This commit also fixes a small typo and adds a TODO to warn users when
delete and blit are both implemented (as typically one shouldn't need
delete if a type is truly blittable).

In other words, the borrow checker will no longer complain about the following:

(deftype Thing Foo Bar)

(defmodule Thing
  (defn blit [x] (the Thing x))
  (implements blit blit)
)

(defn multi-use-thing [] 
  (let-do [thing (Thing.Foo)
           other-thing thing]
    (f thing)
    (g thing)
    (h other-thing)
    (k other-thing)))

That is, there's no need to borrow types that indicate blit. They are assumed to have copy semantics and not require deletion.

This PR also includes a test for this behavior.

scolsen and others added 15 commits November 29, 2021 10:22
This commit adds an implementation of Boxes, memory manged heap
allocated values.

Boxes are implemented as C pointers, with no additional structure but
are treated as structs in Carp. To facilitate this, we need to add them
as a clause to our special type emissions (TypesToC) as they'd otherwise
be emitted like other struct types.

Co-authored-by: Veit Heller <[email protected]>
Make sure we free the box!
Now that a builtin type named Box exists, the definitions in this file
cause a conflict. I've renamed the "Box" type in the functor example to
remove the conflict.
Box.peek allows users to transform a reference to a box into a a
reference to the box's contained value. The returned reference will have
the same lifetime as the box. This function allows callers to manipulate
the value in a box without re-allocation, for example:

```clojure
(deftype Num [val Int])

(let-do [box (Box.init (Num.init 0))]
  (Num.set-val! (Box.peek &box) 1)
  @(Num.val (Box.peek &box)))
```

This commit also includes tests for Box.peek.

Co-authored-by: TimDeve <[email protected]>
The `blit` interface, when implemented, indicates that a type is freely
copyable and does not require deletion. It is similar to Rust's Copy
trait and indicates that a type should have copy instead of "move"
semantics. This commit updates the memory management system to ignore
such types, since we do not need to worry about deleting them.

Thus, when using a blittable type, users can freely use a variable of
the type in more than one place.

This commit also fixes a small typo and adds a TODO to warn users when
delete and blit are both implemented (as typically one shouldn't need
delete if a type is truly blittable).
This commit updates memory.carp to test that types that implement blit
are freely copyable (have copy instead of move semantics, are not borrow
checked) and do not cause leaks.

Of course, users can still cause leaks if they define blit on a type
that isn't actually freely copyable (e.g. it owns some data that is heap
allocated), but this test checks the basic behavior.
@scolsen scolsen requested a review from a team April 1, 2022 14:24
@scolsen
Copy link
Contributor Author

scolsen commented Apr 1, 2022

if this merges I'll open up an issue to produce a warning for types that implement both blit and delete (there's a TODO in this commit for this)

@TimDeve
Copy link
Contributor

TimDeve commented Apr 1, 2022

It would be useful to expose interfaceImplementedForTy as a Dynamic function so that people can derive blit rather than implementing it themselves as it's unsafe. Could be a follow up PR though.

@scolsen
Copy link
Contributor Author

scolsen commented Apr 1, 2022

It would be useful to expose interfaceImplementedForTy as a Dynamic function so that people can derive blit rather than implementing it themselves as it's unsafe. Could be a follow up PR though.

I think this is definitely worth doing. Related to this, perhaps we should add something about manual implementations being unsafe somewhere in the docs

@TimDeve
Copy link
Contributor

TimDeve commented Apr 1, 2022

...perhaps we should add something about manual implementations being unsafe somewhere in the docs

For sure, something like "This is a marker interface. DO NOT implement yourself"

Copy link
Collaborator

@eriksvedang eriksvedang left a comment

Choose a reason for hiding this comment

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

Pretty straight-forward!

test/memory.carp Show resolved Hide resolved
@eriksvedang
Copy link
Collaborator

@scolsen I'll merge this now if that's OK?

@scolsen
Copy link
Contributor Author

scolsen commented Apr 4, 2022

Go for it!

@eriksvedang eriksvedang merged commit 3e5bdc0 into carp-lang:master Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants