-
-
Notifications
You must be signed in to change notification settings - Fork 836
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
feat: add min and max value builtins #2935
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2935 +/- ##
==========================================
- Coverage 88.10% 86.56% -1.54%
==========================================
Files 97 97
Lines 10877 10923 +46
Branches 2574 2522 -52
==========================================
- Hits 9583 9456 -127
- Misses 838 995 +157
- Partials 456 472 +16
Help us with your feedback. Take ten seconds to tell us how you rate us. |
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.
looks nice! i might rename def _eval_fn()
to def _eval()
.
also i think we want to be sure not to actually remove the old constants from the language. We can deprecate them but we should keep them in for a few releases to avoid breaking user code.
docs/constants-and-vars.rst
Outdated
@@ -93,11 +93,6 @@ Name Type Value | |||
================= ================ ============================================== | |||
``ZERO_ADDRESS`` ``address`` ``0x0000000000000000000000000000000000000000`` |
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 mean we might as well get rid of ZERO_ADDRESS
and EMPTY_BYTES32
while we are at it
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 can support min_value(bytesN)
as well since Vyper now has all bytesN
types.
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.
well that's just empty(bytesN)
. actually i think for types T
where min_value(T) == empty(T)
, we should block min_value(T)
and suggest empty(T)
instead (on the principle that there should be one obvious way to do things).
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.
oh right, so empty()
is already sufficient for ZERO_ADDRESS
and EMPTY_BYTESN
.
in that case, the only types where min_value(T) == empty(T)
are unsigned integers only though. hence, I think supporting min_value
for signed integers but not unsigned integers would be more confusing than having min_value
and empty
for unsigned integers.
To discuss replacing |
Notes from meeting on 25 July: proceed with |
meeting notes: |
Do we need to support |
yes, that would be good! |
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.
looking quite good. couple small things
vyper/builtin_functions/functions.py
Outdated
|
||
if isinstance(input_type, IntegerAbstractType): | ||
(lo, hi) = int_bounds(input_type._is_signed, input_type._bits) | ||
val = self._eval(lo, hi) |
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.
it's nice to use this _eval
function. dispatching to min/max
is a bit magical though. for instance, if the signature of int_bounds
ever changes, you would expect this function to break actually - and it wouldn't!
i would say this would be slightly clearer with two functions, _eval_decimal
and _eval_int
. each takes the input type and returns a value. so this block would look more like
if isinstance(input_type, IntegerAbstractType):
val = self._eval_int(input_type)
and similar for the decimal case.
What I did
Fix #1923.
How I did it
Add
min_value
andmax_value
builtin functions.How to verify it
See tests.
Commit message
Description for the changelog
Add min_value and max_value builtin functions.
Cute Animal Picture