-
Notifications
You must be signed in to change notification settings - Fork 196
Overhaul spec content #489
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
cacea03
to
2adc3e3
Compare
Codecov Report
@@ Coverage Diff @@
## master #489 +/- ##
=======================================
Coverage 87.69% 87.69%
=======================================
Files 80 80
Lines 5276 5276
=======================================
Hits 4627 4627
Misses 649 649 Continue to review full report at Codecov.
|
49cc6c5
to
dbcd1e8
Compare
041a48a
to
47c14fa
Compare
@g-r-a-n-t This is far from finished but I think it may be good to get the first chunk in before it grows to large. |
Looks great. I left a bunch of comments, some of which are suggestions and others are just general thoughts that probably warrant new issues. |
6 / 3 == 2 TODO: Rest | ||
5 % 4 == 1 | ||
2 ** 4 == 16 | ||
12 & 25 == 8 |
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 could use the bin representation for bitwise ops
@@ -0,0 +1,3 @@ | |||
# Function calls | |||
|
|||
Constant size values stored on the stack or in memory can be passed into and returned by functions. |
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 might be more clear:
All types, except for
Map
, may be passed into and returned by functions.
square: Rectangle = Rectangle(width=10, length=10) | ||
``` | ||
|
||
All fields of struct types are always initialized. |
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.
All fields of struct types must be initialized.
@@ -0,0 +1,9 @@ | |||
# The `to_mem` function | |||
|
|||
Reference type values can be copied from storage and into memory using the `to_mem` function. |
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.
In the future, I think there may be somewhat nuanced usage of to_mem()
on values in memory.
Example:
pub fn foo(self):
self.sum(self.my_array)
self.sum([1, 2, 3]) # this is not copied in `sum`, as `to_mem` operates on it passively
fn sum(some_array: u8[3]):
let sum: u256 = 0
for val in some_array.to_mem():
sum += val
return sum
So I think we should remove the check requiring that the value be in storage (if it exists). This would of course be a separate issue.
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.
But why would we even write some_array.to_mem()
here? Isn't the assumption that some_array
is in memory anyway? Isn't it that we would only need to call to_mem()
on something that is self.<something>
? Or is this hypothetical code in which some_array
may allow readonly storage pointers?
Also we might want to reconsider if to_mem
can be merged with clone
.
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.
Isn't the assumption that some_array is in memory anyway?
This is the current assumption, as we require that reference type parameters be in memory.
Or is this hypothetical code in which some_array may allow readonly storage pointers?
This is what has been described in #161. to_mem
would serve as a function that would guarantee that a readonly value is in memory. If the value is already in memory, then we would leave it alone, as to not incur unnecessary gas costs. If the value is in storage, then we would copy it.
Also, I'd be happy to rewrite a bunch of stuff in the data layout section after this gets merged. |
47c14fa
to
c44317d
Compare
What was wrong?
How was it fixed?
Still an ongoing effort but: