Skip to content

[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

Merged
merged 1 commit into from
Mar 7, 2019
Merged

Conversation

erak
Copy link
Collaborator

@erak erak commented Mar 5, 2019

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:

  1. Changes hex number literals (e.g. 0x23) such that they are right-aligned (which should be the natural behavior.)
  2. Introduces two new keywords: left and right. These can be used to explicitly define the alignment of values:
contract C {
    uint256 public stateDecimal = 0x20;
}

contract D {
    bool public stateBool = true;
    uint256 public stateDecimal = 42;
    bytes32 public stateBytes = "\x42\x00\xef";

    function internalStateDecimal() public returns (uint256) {
        return (new C()).stateDecimal();
    }

    function update(bool _bool, uint256 _decimal, bytes32 _bytes) public returns (bool, uint256, bytes32) {
        stateBool = _bool;
        stateDecimal = _decimal;
        stateBytes = _bytes;
        return (stateBool, stateDecimal, stateBytes);
    }
}
// ----
// stateBool() -> true
// stateBool() -> right(true)
// stateDecimal() -> 42
// stateDecimal() -> right(42)
// stateBytes() -> left(0x4200ef)
// internalStateDecimal() -> 0x20
// update(bool,uint256,bytes32): false, -23, left(0x2300ef) -> false, -23, left(0x2300ef)

@erak erak changed the title Soltest alignment [soltest] Introduce explicit alignment Mar 5, 2019
@erak erak mentioned this pull request Mar 5, 2019
1 task
@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #6183 into develop will decrease coverage by 0.05%.
The diff coverage is 91.14%.

Impacted file tree graph

@@             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
Flag Coverage Δ
#all 87.92% <91.14%> (-0.06%) ⬇️
#syntax 27.04% <5.29%> (-0.06%) ⬇️

result = isFailure ? failure : formatBytesParameters(output, m_call.expectations.result);
result = isFailure
? failure
: matchesExpectation()
Copy link
Collaborator Author

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.

Copy link
Member

@ekpyron ekpyron left a 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);
Copy link
Member

@ekpyron ekpyron Mar 5, 2019

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.

@erak
Copy link
Collaborator Author

erak commented Mar 6, 2019

Squashed and updated. @ekpyron Took in your suggestions.

@erak
Copy link
Collaborator Author

erak commented Mar 6, 2019

Argh, I put in quite some effort to make Codecov happy. But we're still testing tests here, so I think a delta of -0.03% should be fine ;)

@erak erak force-pushed the soltest-alignment branch from ba68ca7 to a40fbf0 Compare March 6, 2019 17:24
@chriseth chriseth merged commit 0eb7994 into develop Mar 7, 2019
@chriseth chriseth deleted the soltest-alignment branch April 30, 2019 14:01
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