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

Initial implementation of FFI macro and codegen tool #1

Merged
merged 6 commits into from
Jun 22, 2021

Conversation

shadaj
Copy link
Member

@shadaj shadaj commented Jun 17, 2021

No description provided.

@shadaj shadaj requested review from sffc and Manishearth June 17, 2021 18:59
macro/src/lib.rs Outdated Show resolved Hide resolved
@@ -2,16 +2,20 @@
mod ffi {
use fixed_decimal::FixedDecimal;

pub struct ICU4XFixedDecimal(pub FixedDecimal);
pub struct ICU4XFixedDecimal(pub Box<FixedDecimal>);
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually don't think we should be doing this, because this adds an extra layer of indirection for the &self methods.

I'd do #[repr(transparent)] pub struct ICU4XFixedDecimal(pub FixedDecimal);and havenew()return aBox`

Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Aside from the Box thing, none of my suggestions are blocking, so we should be able to land this as a first pass if you'd like

.as_ref()
.unwrap()
.1
.iter()
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (nb): I'd imagine that you want to collect the struct def too and store it alongside the methods (especially as we start having attributes on struct defs)

#[derive(Debug)]
pub struct Method {
pub name: String,
pub full_path_name: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

chore (nb): comments explaining what these fields are, especially this one

core/src/meta.rs Outdated Show resolved Hide resolved
}

#[derive(Clone, Debug)]
pub struct Type {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (nb): eventually should be a nestable enum

macro/src/lib.rs Outdated Show resolved Hide resolved
println!("{}", out.join("\n"));
}

fn main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (nb): For experimentation it seems fair to directly do JS, but eventually we probably want the tool itself to just spit out JSON.

}

fn main() {
let lib_file = syn_inline_mod::parse_and_inline_modules(&Path::new("./src/main.rs"));
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (nb): diplomat-core should probably export both:

  • a way to get module+struct tree from a full syn crate
  • a way to get a module+struct tree from a lib.rs root

tool/src/main.rs Outdated Show resolved Hide resolved
@Manishearth
Copy link
Contributor

Here's what the expanded code looks like so far!

$ cargo rustc -- -Zunpretty=expanded
   Compiling example v0.1.0 (/home/manishearth/dev/diplomat/example)
#![feature(prelude_import)]
#[prelude_import]
use std::prelude::rust_2018::*;
#[macro_use]
extern crate std;
mod ffi {
    use fixed_decimal::FixedDecimal;

    #[repr(transparent)]
    pub struct ICU4XFixedDecimal(pub FixedDecimal);

    impl ICU4XFixedDecimal {
        fn new(v: i32) -> Box<ICU4XFixedDecimal> {
            Box::new(ICU4XFixedDecimal(FixedDecimal::from(v)))
        }

        fn multiply_pow10(&mut self, power: i16) {
            self.0.multiply_pow10(power).unwrap();
        }

        fn digit_at(&self, magnitude: i16) -> u8 {
            self.0.digit_at(magnitude)
        }
    }
    #[no_mangle]
    pub extern "C" fn ICU4XFixedDecimal_new(v: i32)
     -> Box<ICU4XFixedDecimal> {
        ICU4XFixedDecimal::new(v)
    }
    #[no_mangle]
    pub extern "C" fn ICU4XFixedDecimal_multiply_pow10(this:
                                                           &mut ICU4XFixedDecimal,
                                                       power: i16) {
        this.multiply_pow10(power);
    }
    #[no_mangle]
    pub extern "C" fn ICU4XFixedDecimal_digit_at(this: &ICU4XFixedDecimal,
                                                 magnitude: i16) -> u8 {
        this.digit_at(magnitude)
    }
}

fn main() {
    let mut decimal = ffi::ICU4XFixedDecimal_new(123);
    ffi::ICU4XFixedDecimal_multiply_pow10(&mut decimal, -1);
}
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s

Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Ideally, for -> Box<Foo> we should have a different return type, and in the C code we should translate that to -> *mut Foo. But doesn't have to be done yet.

Anyway, this should be landable

@shadaj shadaj changed the title Initial macro for generating extern C functions Initial implementation of FFI macro and codegen tool Jun 22, 2021
@shadaj shadaj marked this pull request as ready for review June 22, 2021 00:43
Copy link
Contributor

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Non-blocking review from me; feel free to merge once Manish's comments have been addressed!

(Also looking forward to when you can add documentation.)

pub struct ICU4XFixedDecimal(pub FixedDecimal);

impl ICU4XFixedDecimal {
fn new(v: i32) -> Box<ICU4XFixedDecimal> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I remember in our review of this earlier, you expressed intent to move the Box into the struct

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I asked for it to be reversed: this makes reference semantics worse

@Manishearth Manishearth merged commit 6916ec9 into rust-diplomat:main Jun 22, 2021
Walter-Reactor pushed a commit to Walter-Reactor/diplomat that referenced this pull request Jan 27, 2025
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.

3 participants