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

refactor: FFI #7

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

refactor: FFI #7

wants to merge 9 commits into from

Conversation

lxl66566
Copy link
Collaborator

@lxl66566 lxl66566 commented Oct 11, 2024

This PR is a recommitted PR, split from total modification.

  • delete unneeded files.
  • move ffi code from utils.rs to ffi/mod.rs
  • Implement NsRegister to execute any clojure code from user.
  • add serde.clj, it provides customized ser/de methods. It's a fixup of serialization. (close Clojure ser/de inconsistency #10 )
    • The rust-clojure ffi serialization/deserialization has some unexpected behavior:
      • missing : for keytype objects. For example, {:a :b} will be translated to {a b} by org.clojure/data.json.
      • Even if setting :key-fn keyword, the result will still be translated to {:a b}, the keytype is only for keys, not values.
      • cheshire/cheshire has the same performance
  • remove unneeded test in lib.rs. The previous tests in lib.rs were used to verify feasibility and are now temporarily deleted, after which they will be moved to the accordding modules.

assert_eq!(gen.alloc_new_generator(rt.create_node().build()), 1);
}
}
mod tests {}

Choose a reason for hiding this comment

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

Why do you remove these tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This pattern has been discarded. #9 shows the new generator id allocation scheme.

@@ -164,6 +164,16 @@ mod tests {
use super::*;
use crate::{cljeval, init_jvm};

/// We can define a function in namespace, and call it later.
#[test]
fn test_defn_in_ns() -> Result<(), Box<dyn std::error::Error>> {

Choose a reason for hiding this comment

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

This test case is meaningless because you print the y, not asserting it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a test shows that two cljeval!() will share the same namespace.
If the hypothesis is not true, the jvm will panic so this test will fail.

Do you think the function name or the comment need to be changed?

src/ffi/register.rs Outdated Show resolved Hide resolved
src/clojure/serde.clj Show resolved Hide resolved
src/clojure/serde.clj Outdated Show resolved Hide resolved
src/clojure/serde.clj Outdated Show resolved Hide resolved
}

/// print a clojure instance
pub fn print_clj(inst: Instance) {
Copy link

Choose a reason for hiding this comment

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

Why do we need a print_clj when we already have print?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In some case, a clojure structure may not be print prettily by java.lang.System.out, it would be printed like CustomObject@6d06d69c.

But I'm sorry I cannot reproduce that currently

with_jvm(|jvm| -> jResult<_> { jvm.to_rust(jvm.cast(inst, "java.lang.String")?) })
}

/// This trait is for printing error messages better than `unwrap()`
Copy link

Choose a reason for hiding this comment

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

Use expect ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

except()/unwrap() will print message like that:

thread 'ffi::tests::mytest' panicked at src/ffi/mod.rs:248:49:
called `Result::unwrap()` on an `Err` value: JavaError("java.lang.IllegalStateException: Attempting to call unbound fn: #'jepsen.history/histo\n\tat clojure.lang.Var$Unbound.throwArity(Var.java:45)\n\tat clojure.lang.AFn.invoke(AFn.java:32)\n\tat clojure.lang.Var.invoke(Var.java:384)\n\tat java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)\n\tat java.base/java.lang.reflect.Method.invoke(Method.java:580)\n\tat org.astonbitecode.j4rs.api.invocation.JsonInvocationImpl.invokeMethod(JsonInvocationImpl.java:198)\n\tat org.astonbitecode.j4rs.api.invocation.JsonInvocationImpl.invoke(JsonInvocationImpl.java:70)\n")

The java error is wrapped as String by j4rs.

update: I modified the die function to print better string.

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.

Clojure ser/de inconsistency
3 participants