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(interface-types) Add the binary encoder #1216

Merged
merged 5 commits into from
Feb 18, 2020

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Feb 14, 2020

This PR adds the encoders::binary module, which exposes the ToBytes trait. It is used to encode the AST into the WIT binary representation.

Check the tests to get examples, but quickly, the roundtrip works:

fn test_binary_encoding_decoding_roundtrip() {
    // Let `original_ast` be an AST representing a set of WIT interfaces
    let original_ast = Interfaces {
        exports: vec![Export {
            name: "ab",
            input_types: vec![InterfaceType::I32],
            output_types: vec![InterfaceType::I32],
        }],
        types: vec![Type::new(
            "ab",
            vec!["cd", "e"],
            vec![InterfaceType::I32, InterfaceType::I32],
        )],
        imports: vec![Import {
            namespace: "a",
            name: "b",
            input_types: vec![InterfaceType::I32],
            output_types: vec![InterfaceType::I64],
        }],
        adapters: vec![Adapter::Import {
            namespace: "a",
            name: "b",
            input_types: vec![InterfaceType::I32],
            output_types: vec![InterfaceType::I32],
            instructions: vec![Instruction::ArgumentGet { index: 1 }],
        }],
        forwards: vec![Forward { name: "a" }],
    };

    // Let's encode the AST into the WIT binary representation.
    let mut binary = vec![];

    original_ast
        .to_bytes(&mut binary)
        .expect("Failed to encode the AST.");

    // And let's go back to the AST land.
    let (remainder, ast) = parse::<()>(binary.as_slice()).expect("Failed to decode the AST.");

    assert!(remainder.is_empty());

    // They must equal.
    assert_eq!(original_ast, ast);
}

The implementation with the ToBytes trait and the io::Write trait is —I hope— Rust idiomatic. I reckon the code is easy to read and understand.

@Hywan Hywan self-assigned this Feb 14, 2020
@Hywan Hywan force-pushed the feat-interface-types-encoders-binary branch from ba712e0 to 306d192 Compare February 17, 2020 12:57
@Hywan Hywan marked this pull request as ready for review February 17, 2020 13:08
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

Looks good to me!

We could probably use something like serde here but 🤷‍♂ , I think it's fine!

@Hywan
Copy link
Contributor Author

Hywan commented Feb 18, 2020

bors r+

bors bot added a commit that referenced this pull request Feb 18, 2020
1216: feat(interface-types) Add the binary encoder r=Hywan a=Hywan

This PR adds the `encoders::binary` module, which exposes the `ToBytes` trait. It is used to encode the AST into the WIT binary representation.

Check the tests to get examples, but quickly, the roundtrip works:

```rust
fn test_binary_encoding_decoding_roundtrip() {
    // Let `original_ast` be an AST representing a set of WIT interfaces
    let original_ast = Interfaces {
        exports: vec![Export {
            name: "ab",
            input_types: vec![InterfaceType::I32],
            output_types: vec![InterfaceType::I32],
        }],
        types: vec![Type::new(
            "ab",
            vec!["cd", "e"],
            vec![InterfaceType::I32, InterfaceType::I32],
        )],
        imports: vec![Import {
            namespace: "a",
            name: "b",
            input_types: vec![InterfaceType::I32],
            output_types: vec![InterfaceType::I64],
        }],
        adapters: vec![Adapter::Import {
            namespace: "a",
            name: "b",
            input_types: vec![InterfaceType::I32],
            output_types: vec![InterfaceType::I32],
            instructions: vec![Instruction::ArgumentGet { index: 1 }],
        }],
        forwards: vec![Forward { name: "a" }],
    };

    // Let's encode the AST into the WIT binary representation.
    let mut binary = vec![];

    original_ast
        .to_bytes(&mut binary)
        .expect("Failed to encode the AST.");

    // And let's go back to the AST land.
    let (remainder, ast) = parse::<()>(binary.as_slice()).expect("Failed to decode the AST.");

    assert!(remainder.is_empty());

    // They must equal.
    assert_eq!(original_ast, ast);
}
```

The implementation with the `ToBytes` trait and the `io::Write` trait is —I hope— Rust idiomatic. I reckon the code is easy to read and understand.

Co-authored-by: Ivan Enderlin <[email protected]>
Co-authored-by: Ivan Enderlin <[email protected]>
@Hywan
Copy link
Contributor Author

Hywan commented Feb 18, 2020

I'm not sure serde will be faster, but the implementation could be simpler, maybe. Not sure. It's one less dependency. As you said, 🤷‍♂.

@bors
Copy link
Contributor

bors bot commented Feb 18, 2020

Build succeeded

@bors bors bot merged commit cc93f31 into wasmerio:master Feb 18, 2020
@MarkMcCaskey
Copy link
Contributor

I'm not sure serde will be faster, but the implementation could be simpler, maybe. Not sure. It's one less dependency. As you said, 🤷‍♂.

Yeah, I don't think it'll be faster and it'll certainly be more complex if we have to implement the serde traits ourselves (I've done this 2-3 times and it's always orders of magnitude more complex than I expect... which probably means that I'm rushing ahead to implement it without understanding it first...), I think serde helps with flexibility and convenience when derive works, but I agree for now it's unnecessary!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 documentation Do you like to read? 🎉 enhancement New feature! 🧪 tests I love tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants