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.

Sign up for GitHub

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

Jump to bottom

test+doc(c-api): Test and document the C A PI #1851

Merged
merged 20 commits into from
Dec 11, 2020
Merged

test+doc(c-api): Test and document the C A PI #1851

merged 20 commits into from
Dec 11, 2020

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Nov 30, 2020 •
edited
Loading

Description

This PR improves the test suites and the documentation of the C API.

Review

  • Add a short description of the the change to the CHANGELOG.md file

@Hywan Hywan added 📦 lib-c-api About wasmer-c-api 🧪 tests I love tests 📚 documentation Do you like to read? labels Nov 30, 2020
@Hywan Hywan self-assigned this Nov 30, 2020
jubianchi
jubianchi reviewed Nov 30, 2020
View reviewed changes
lib/c-api/src/wasm_c_api/engine.rs Show resolved Hide resolved
lib/c-api/src/wasm_c_api/engine.rs Show resolved Hide resolved
lib/c-api/src/wasm_c_api/engine.rs
use inline_c::assert_c;

#[test]
fn test_engine_new() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this test really useful? It seems redundant with the examples from the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially wanted to improve this test, but our C API doesn't offer enough features. Let's keep this as is, and I'll improve it later.

lib/c-api/src/wasm_c_api/instance.rs Show resolved Hide resolved
Hywan added 4 commits December 1, 2020 11:37
@Hywan
Merge branch 'master' into test-c-api
f2a2cf4
@Hywan
Merge branch 'master' into test-c-api
8899509
@Hywan
fix(c-api) Fix memory leak in wasm_$name_vec_delete.
527b706 
This patch does several things:

1. It applies our Rust patterns for C API by replacing the raw pointer
   by `Option<Box<T>>`,
2. It allows `wasm_$name_vec_delete` to handle null pointer,
3. Because it takes ownership of the `wasm_$name_vec_t`, the pointer
   is correctly dropped (which fix the memory leak).
@Hywan
fixup
25bcc9b
@jubianchi jubianchi added the 1.0 Wasmer at 1.0 label Dec 4, 2020
@Hywan Hywan marked this pull request as ready for review December 10, 2020 15:19
@Hywan
Copy link
Contributor Author

Hywan commented Dec 10, 2020

bors try

bors bot added a commit that referenced this pull request Dec 10, 2020
@bors
Try #1851:
1c8f730
@bors
Copy link
Contributor

bors bot commented Dec 10, 2020

try

Build failed:

@Hywan
Copy link
Contributor Author

Hywan commented Dec 10, 2020

bors try

bors bot added a commit that referenced this pull request Dec 10, 2020
@bors
Try #1851:
4df763a
@bors
Copy link
Contributor

bors bot commented Dec 10, 2020

try

Build failed:

@Hywan
Copy link
Contributor Author

Hywan commented Dec 10, 2020

So. I pushed a commit. Git says everything is up-to-date. But the commit doesn't appear here. Hmmm.

@Hywan
Copy link
Contributor Author

Hywan commented Dec 10, 2020

bors try

bors bot added a commit that referenced this pull request Dec 10, 2020
@bors
Try #1851:
442d1ec
@bors
Copy link
Contributor

bors bot commented Dec 10, 2020

try

Build failed:

@jubianchi
test(c-api) Add no_run to engine tests.
401b9e4 
Because we test the C API with many features combination, we can't ensure tests will pass with every combination
@jubianchi
chore: update wamser_wasm.h
62fa2e5
@jubianchi
Copy link
Contributor

bors try

bors bot added a commit that referenced this pull request Dec 10, 2020
@bors
Try #1851:
613188c
@bors
Copy link
Contributor

bors bot commented Dec 10, 2020

try

Build succeeded:

@Hywan
Copy link
Contributor Author

Hywan commented Dec 11, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 11, 2020

Build succeeded:

@bors bors bot merged commit 4f174f8 into wasmerio:master Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jubianchi jubianchi jubianchi approved these changes

@nlewycky nlewycky Awaiting requested review from nlewycky

@MarkMcCaskey MarkMcCaskey Awaiting requested review from MarkMcCaskey

Assignees

@Hywan Hywan

Labels
1.0 Wasmer at 1.0 📚 documentation Do you like to read? 📦 lib-c-api About wasmer-c-api 🧪 tests I love tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Hywan @jubianchi