-
Notifications
You must be signed in to change notification settings - Fork 6.1k
[soltest] Introduce explicit alignment #6183
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
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #6183 +/- ##
===========================================
- Coverage 87.97% 87.92% -0.06%
===========================================
Files 377 377
Lines 36221 36191 -30
Branches 4286 4279 -7
===========================================
- Hits 31867 31822 -45
- Misses 2904 2913 +9
- Partials 1450 1456 +6
|
result = isFailure ? failure : formatBytesParameters(output, m_call.expectations.result); | ||
result = isFailure | ||
? failure | ||
: matchesExpectation() |
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.
This is an important change that was needed to get the update in isoltest right if left()
or right()
was used to define the expected result.
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.
One suggestion which can be addressed in varying intensity ;-). Otherwise this should be squashed (looking at the individual commits first was a bit misleading ;-)), but generally looks good to me!
auto applyAlign = [&](DeclaredAlignment _modifier, bytes _bytes) -> pair<bytes, ABIType::Align> | ||
{ | ||
if (_modifier == DeclaredAlignment::Right) | ||
return make_pair(alignRight(_bytes), ABIType::AlignRight); |
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 could probably inline alignRight
(and alignLeft
below). I also wonder whether skipping the whole DeclaredAlignment
business and using a capture-less alignment
lambda instead, supplemented by a hasAlignModifier
flag for the alignment != Alignment::None
check below might be nicer. Probably fine either way, though.
Also you could pass in a reference to ABIType
and set its align
member in the lambda instead of returning a pair - that might spare you reassigning ABIType::align
over and over below. For that matter it could also just be a ref to bytes
to which the padding is appended/prepended.
Squashed and updated. @ekpyron Took in your suggestions. |
Argh, I put in quite some effort to make Codecov happy. But we're still testing tests here, so I think a delta of |
As it turned out, we implimented the alignment of hex number literals in a confusing way (#6120 (comment)). This PR does the following two things:
0x23
) such that they are right-aligned (which should be the natural behavior.)left
andright
. These can be used to explicitly define the alignment of values: