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

capi: imported and exported tables #628

Merged
merged 3 commits into from
Nov 3, 2020
Merged

capi: imported and exported tables #628

merged 3 commits into from
Nov 3, 2020

Conversation

gumb0
Copy link
Collaborator

@gumb0 gumb0 commented Oct 29, 2020

No description provided.

@codecov
Copy link

codecov bot commented Oct 29, 2020

Codecov Report

Merging #628 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #628      +/-   ##
==========================================
+ Coverage   98.35%   98.36%   +0.01%     
==========================================
  Files          69       69              
  Lines        9526     9592      +66     
==========================================
+ Hits         9369     9435      +66     
  Misses        157      157              

@gumb0 gumb0 force-pushed the capi-external-table branch 3 times, most recently from 0392a38 to b0336db Compare November 2, 2020 12:01
@gumb0 gumb0 marked this pull request as ready for review November 2, 2020 12:02
@gumb0 gumb0 changed the title capi: Instantiate with imported table capi: imported and exported tables Nov 2, 2020
@gumb0 gumb0 requested review from chfast and axic November 2, 2020 12:50
include/fizzy/fizzy.h Outdated Show resolved Hide resolved
@gumb0 gumb0 force-pushed the capi-external-table branch 2 times, most recently from 036b86a to d8a6174 Compare November 3, 2020 11:31
@gumb0
Copy link
Collaborator Author

gumb0 commented Nov 3, 2020

Rebased.

inline std::vector<fizzy::ExternalTable> unwrap(const FizzyExternalTable* external_table)
{
return external_table ? std::vector<fizzy::ExternalTable>{unwrap(*external_table)} :
std::vector<fizzy::ExternalTable>{};
Copy link
Member

@axic axic Nov 3, 2020

Choose a reason for hiding this comment

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

Cannot just {} work here? And same with {unwrap(...)}?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, error: initializer list cannot be used on the right hand side of operator '?'

/// @param name The table name. NULL-terminated string. Cannot be NULL.
/// @param out_table Pointer to output struct to store found table. Cannot be NULL.
/// @returns true if table was found, false otherwise.
bool fizzy_find_exported_table(
Copy link
Member

@axic axic Nov 3, 2020

Choose a reason for hiding this comment

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

I was going to say why isn't this grouped with find_exported_function, but realised that is over the module. This still confuses me in our API, both C++ and in C.

I think perhaps it would be clearer all these are instance related and later we could add some module inspection features.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I put them separately, because this one operates on instance.

The change in #633 adds another fizzy_find_exported_function, that operates on instance and would be convenient, when you need to get an exported function and import it into another instance. With it there's going to be 4 similar fizzy_find_exported_* as in C++ API.
(However it has some complications with managing the lifetime of capture, so I'm not completely sure it's a good idea)
The function operating on module is renamed there to fizzy_find_exported_function_index, which better reflects what it does.

(module
(func $f (export "foo") (result i32) (i32.const 42))
(global (export "g1") i32 (i32.const 42))
(table (export "tab") 10 30 anyfunc)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add another table without a maximum?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added.

@gumb0 gumb0 mentioned this pull request Nov 3, 2020
49 tasks
@gumb0 gumb0 force-pushed the capi-external-table branch 2 times, most recently from 3e98f7e to aca8ba0 Compare November 3, 2020 16:11
@gumb0 gumb0 force-pushed the capi-external-table branch from aca8ba0 to 3b51d9f Compare November 3, 2020 17:13
@gumb0 gumb0 merged commit 2bee307 into master Nov 3, 2020
@gumb0 gumb0 deleted the capi-external-table branch November 3, 2020 18:08
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