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

Drop default depth param from execute() #756

Merged
merged 3 commits into from
Mar 19, 2021
Merged

Drop default depth param from execute() #756

merged 3 commits into from
Mar 19, 2021

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Mar 12, 2021

In preparation for landing ExecutionContext, the default value for depth param is dropped in C++ and the param is completely removed from C and Rust (therefore C and Rust are not able to legally call wasm functions from host functions temporarily).

@codecov
Copy link

codecov bot commented Mar 12, 2021

Codecov Report

Merging #756 (02afded) into master (5d09b85) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #756      +/-   ##
==========================================
- Coverage   99.25%   99.25%   -0.01%     
==========================================
  Files          76       76              
  Lines       11574    11567       -7     
==========================================
- Hits        11488    11481       -7     
  Misses         86       86              
Flag Coverage Δ
rust 99.86% <100.00%> (-0.01%) ⬇️
spectests 90.55% <100.00%> (+0.01%) ⬆️
unittests 99.21% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
bindings/rust/src/lib.rs 99.86% <100.00%> (-0.01%) ⬇️
lib/fizzy/capi.cpp 99.27% <100.00%> (ø)
lib/fizzy/execute.hpp 100.00% <100.00%> (ø)
test/unittests/capi_test.cpp 99.87% <100.00%> (ø)
test/utils/fizzy_c_engine.cpp 100.00% <100.00%> (ø)

bindings/rust/src/lib.rs Outdated Show resolved Hide resolved
bindings/rust/src/lib.rs Outdated Show resolved Hide resolved
bindings/rust/src/lib.rs Outdated Show resolved Hide resolved
Instance& instance, FuncIdx func_idx, const Value* args, int depth = 0) noexcept;
Instance& instance, FuncIdx func_idx, const Value* args, int depth) noexcept;

/// Execute a function from an instance starting at default depth of 0.
Copy link
Member

Choose a reason for hiding this comment

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

I'd add "Arguments and behaviour is the same as execute."

/// Execute a function from an instance starting at default depth of 0.
inline ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args) noexcept
{
const int depth = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure it makes a difference, but constexpr?

Copy link
Member

@axic axic 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, but the doxygen improvement should be applied.

@chfast chfast merged commit e9b24a7 into master Mar 19, 2021
@chfast chfast deleted the depth_param branch March 19, 2021 16:43
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