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: Type and global inspection functions #675

Merged
merged 2 commits into from
Jan 11, 2021
Merged

Conversation

gumb0
Copy link
Collaborator

@gumb0 gumb0 commented Dec 17, 2020

No description provided.

@codecov
Copy link

codecov bot commented Dec 17, 2020

Codecov Report

Merging #675 (8f7ff15) into master (26bb712) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master     #675    +/-   ##
========================================
  Coverage   99.31%   99.31%            
========================================
  Files          72       72            
  Lines       10148    10257   +109     
========================================
+ Hits        10078    10187   +109     
  Misses         70       70            
Flag Coverage Δ
spectests 91.50% <ø> (ø)
unittests 99.31% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/fizzy/capi.cpp 98.72% <100.00%> (+0.04%) ⬆️
test/unittests/capi_test.cpp 100.00% <100.00%> (ø)

@gumb0 gumb0 force-pushed the capi-module-inspection branch 8 times, most recently from 7ffaf26 to 761bf6e Compare December 23, 2020 12:19
@gumb0 gumb0 marked this pull request as ready for review December 23, 2020 12:29
@axic axic added this to Review in progress in 0.7.0 Dec 24, 2020
include/fizzy/fizzy.h Outdated Show resolved Hide resolved
/// @param module Pointer to module. Cannot be NULL.
uint32_t fizzy_get_global_count(const FizzyModule* module);

/// Get type of the global defined in the module.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Get type of the global defined in the module.
/// Get type of a given global defined in the module.

include/fizzy/fizzy.h Outdated Show resolved Hide resolved

/// Import definition.
///
/// @note Only one member of desc union corresponding to kind is defined for each import.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// @note Only one member of desc union corresponding to kind is defined for each import.
/// @note Only one member of `desc` union corresponding to kind is defined for each import.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this helps in doxygen.

/// Import definition.
///
/// @note Only one member of desc union corresponding to kind is defined for each import.
typedef struct FizzyImport
Copy link
Member

Choose a reason for hiding this comment

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

Since this is only used for inspection, perhaps I would call this something like FizzyImportDetails. The idea is to distinguish this from the regular API used for linking/execution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FizzyImportType perhaps?

include/fizzy/fizzy.h Outdated Show resolved Hide resolved
include/fizzy/fizzy.h Outdated Show resolved Hide resolved
include/fizzy/fizzy.h Outdated Show resolved Hide resolved
test/unittests/capi_test.cpp Outdated Show resolved Hide resolved
test/unittests/capi_test.cpp Outdated Show resolved Hide resolved
@gumb0 gumb0 marked this pull request as draft January 4, 2021 15:17
@gumb0 gumb0 force-pushed the capi-module-inspection branch 7 times, most recently from 26eaea1 to 1763c1d Compare January 5, 2021 18:33
/// Import definition.
///
/// @note Only one member of `desc` union corresponding to `kind` is defined for each import.
typedef struct FizzyImportType
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is type, but perhaps "description" is a better term?

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 the spec it is called just import, and importdesc is functype | tabletype | memtype | globaltype (i.e. everything in import except module and name)

Copy link
Collaborator Author

@gumb0 gumb0 Jan 6, 2021

Choose a reason for hiding this comment

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

In wasm-c-api it is importtype, consisting of module, name and externtype
https://github.com/WebAssembly/wasm-c-api/blob/fd09246781d8dbf6e9491c90924667f78e9dc6e7/include/wasm.h#L285-L294

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also FizzyImportType can be seen as consistent with other module definition types: FizzyFunctionType, FizzyGlobalType (memory and table don't define *Type beacuse they use FizzyLimits directly as a type)

@gumb0 gumb0 marked this pull request as ready for review January 6, 2021 11:46
@gumb0
Copy link
Collaborator Author

gumb0 commented Jan 7, 2021

Import inspection moved to #683

@gumb0 gumb0 changed the title capi: Module inspection functions capi: Type and global inspection functions Jan 7, 2021
@gumb0 gumb0 requested review from chfast and axic January 7, 2021 12:08
@gumb0 gumb0 mentioned this pull request Jan 7, 2021
49 tasks
0.7.0 automation moved this from Review in progress to Reviewer approved Jan 8, 2021
@gumb0 gumb0 merged commit 63ed27c into master Jan 11, 2021
0.7.0 automation moved this from Reviewer approved to Done Jan 11, 2021
@gumb0 gumb0 deleted the capi-module-inspection branch January 11, 2021 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
0.7.0
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants