-
Notifications
You must be signed in to change notification settings - Fork 1.6k
use buffers for uint32 WASM params #6291
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
base: ripple/wasmi-host-functions
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## ripple/wasmi-host-functions #6291 +/- ##
===========================================================
Coverage 80.4% 80.4%
===========================================================
Files 854 854
Lines 67574 67588 +14
Branches 7313 7297 -16
===========================================================
+ Hits 54319 54346 +27
+ Misses 13255 13242 -13
🚀 New features to boost your workflow:
|
| return result; | ||
|
|
||
| return x < 0 ? x : (x >= 5 ? x : 0); | ||
| return sqn >= 5 ? (int32_t)sqn : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We make changes to support values which are greater than INT_MAX, so you should return 1 here, not sqn
sqn >= 5 ? 1 : 0;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (BEAST_EXPECT(result.has_value())) | ||
| BEAST_EXPECT(result.value() == env.current()->parentCloseTime().time_since_epoch().count()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should fix the tests, not just delete them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are no longer relevant. We no longer have code there to check. There is no longer any such error case in these functions.
| } | ||
|
|
||
| template <class IW> | ||
| Expected<uint64_t, HostFunctionError> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uint32_t
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| auto const seq = getDataUInt32(runtime, params, index); | ||
| if (!seq) | ||
| { | ||
| return hfResult(results, seq.error()); // LCOV_EXCL_LINE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing // LCOV_EXCL_LINE - Did you make some tests that can reach this point ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning to but didn't get around to it yesterday. Will do that today.
High Level Overview of Change
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)