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(transient-vector): adds initial support for transient vector #60

Merged
merged 3 commits into from
Mar 15, 2024

Conversation

Samy-33
Copy link
Contributor

@Samy-33 Samy-33 commented Mar 8, 2024

No description provided.

@Samy-33 Samy-33 force-pushed the transient-vector branch from a5e5254 to 67d4771 Compare March 8, 2024 17:39
@Samy-33
Copy link
Contributor Author

Samy-33 commented Mar 8, 2024

I noticed that we don't have a call in persistent_vector. Any reason for that?

In clojure

([1 2 3] 5)

throws and IndexOutOfBoundException, we should have the same behaviour or we can prefer nil punning here? Any reason why clojure can't just return nil instead of throwing exception?
I prefer nil punning personally, but is it valuable enough to divert from clojure's behaviour?

Also, vector is not a 2-arity fn in clojure, but we have declared it in jank. It makes sense to me to have it, but again diverting from clojure's behaviour.

Let me know your thoughts on this, @jeaye

@jeaye
Copy link
Member

jeaye commented Mar 8, 2024

I noticed that we don't have a call in persistent_vector. Any reason for that?

In clojure

([1 2 3] 5)

throws and IndexOutOfBoundException, we should have the same behaviour or we can prefer nil punning here? Any reason why clojure can't just return nil instead of throwing exception? I prefer nil punning personally, but is it valuable enough to divert from clojure's behaviour?

Also, vector is not a 2-arity fn in clojure, but we have declared it in jank. It makes sense to me to have it, but again diverting from clojure's behaviour.

Let me know your thoughts on this, @jeaye

Yeah, we just haven't implemented that behavior on persistent_vector yet. However, the way that it's done is a little tricky, since we actually are hard-coding this right now, in dynamic_call. It's a huge performance win to do this, compared to implementing callable for those types, since that will result in vtable ptrs and virtual dispatch. If you'd like to add support for persistent_vector in there, take a look at callable.cpp:70, as well as evaluate.cpp:260.

As a rule, if Clojure throws, jank throws. We're in the business of hitting parity, not changing semantics (even when we don't necessarily like those semantics).

@jeaye
Copy link
Member

jeaye commented Mar 8, 2024

Some comments for things to improve, but overall this is great! Be sure to test it all in ./build/jank repl, since we don't yet have a test suite for these things.

@Samy-33
Copy link
Contributor Author

Samy-33 commented Mar 9, 2024

Tests:

➜  jank git:(transient-vector) ✗ ./build/jank repl
Bottom of clojure.core
> (def a (transient [1 2 3]))
#'clojure.core/a
> (count a)
3
> (get a 1)
2
> (a 1)
2
> (get a -1)
nil
> (a -1)
Exception: Index out of bound; index = -1, count = 3
> (persistent! a)
[1 2 3]
> (count a)
libc++abi: terminating due to uncaught exception of type std::runtime_error: transient used after it's been made persistent
[1]    94093 abort      ./build/jank repl
➜  jank git:(transient-vector) ✗ git status

namespace jank::runtime
{
obj::transient_vector::static_object(runtime::detail::native_persistent_vector &&d)
: data{ std::move(d).transient() }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This, because of LSP warning about d not getting moved.

@jeaye jeaye merged commit 79e9275 into jank-lang:transient Mar 15, 2024
2 of 4 checks passed
@Samy-33 Samy-33 deleted the transient-vector branch March 15, 2024 19:09
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.

2 participants