feat[codegen]: changed the convert from bytes#5020
Conversation
This reverts commit 6502c1c.
Gas ChangesNo changes detected. Summary
|
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5020 +/- ##
==========================================
+ Coverage 92.92% 92.95% +0.02%
==========================================
Files 188 188
Lines 27820 27857 +37
Branches 4828 4835 +7
==========================================
+ Hits 25853 25894 +41
+ Misses 1316 1311 -5
- Partials 651 652 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| @_input_types(IntegerT) | ||
| def to_flag(expr, arg, out_typ): | ||
| def to_flag(expr, arg, in_typ, out_typ): | ||
| if arg.typ != UINT256_T: |
There was a problem hiding this comment.
When is arg.typ different from in_typ ?
If the answer is "sometimes", this feels like something which should be fixed in the typer
There was a problem hiding this comment.
It should be only in the case the constant.
FOO: constant(Bytes[2]) = b'\xff'
@external
def foo() -> int16:
return convert(FOO, int16)
There was a problem hiding this comment.
In this example, what are the arg.typ and in_typ ?
There was a problem hiding this comment.
arg.typ would be Bytes[1] and and in_typ would be Bytes[2], since the argument is reduced to the b'\xff' at the start.
There was a problem hiding this comment.
There was a problem hiding this comment.
I think it would be better to have arg.typ be Bytes[2], which makes sense since the arg is FOO and its type is Bytes[2]
There was a problem hiding this comment.
That would be some where in the typechecker I think. Do you think it should be done like that? Since there was discussion on what semantics the convert should have. Or maybe I could reduce it later in convert
There was a problem hiding this comment.
Note that your implementation also assumes a specific semantics !
I'm not sure what the semantics of convert should be, but I am 100% sure the following should be equivalent:
FOO: constant(Bytes[2]) = b'\xff'
FOO: constant(Bytes[2]) = b'\x00\xff'Both FOOs should have type Bytes[2] even if their expression has a stricter type.
So we should fix this where appropriate, probably in the typer, since this is not related only to conversions.
(Conversions might be the only place for now where it produces an observable difference however)
There was a problem hiding this comment.
Wait, what I said is wrong, I am 100% sure the following should be equivalent:
FOO: constant(Bytes[2]) = b'\xff'
_FOO: constant(Bytes[1]) = b'\xff'
FOO: constant(Bytes[2]) = _FOOBoth FOOs should have type Bytes[2].
But in my previous example, both should still have type Bytes[2]
|
Does this bring venom towards current vyper, or away from it ? |
|
Please add a test comparing |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ) | ||
| from vyper.semantics.types.bytestrings import _BytestringT | ||
| from vyper.semantics.types.infinity import is_bounded_length | ||
| from vyper.semantics.types.primitives import NumericT |
3 participants
What I did
changed behavior of
convertwhen converting from bytes to intHow I did it
Added step before sign extend so that the value passed is converting from max size and not from runtime size (example in tests).
How to verify it
test added in this PR
Commit message
Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)
Description for the changelog
Cute Animal Picture