-
Notifications
You must be signed in to change notification settings - Fork 121
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: new functions in stdlib from stdlib.fc
and math.fc
#986
base: main
Are you sure you want to change the base?
Conversation
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.
Sorry for reviewing the draft, I just wanted to keep those notes around :)
178e712
to
b9b6206
Compare
@novusnota @anton-trunov Implemented all functions. I'd like to hear some intermediate feedback on them before I proceed with writing tests. I made sure to include all missing functions from both stdlib.fc and mathlib.fc with some exceptions (see PR description). |
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.
afair, mathlib.fc
has lots of functions for fixed-arithmetic as well, I think we should add those too (perhaps as a separate library, like fixed_point_math.tact
).
Also, I left a couple comments in the source code
stdlib/std/cells.tact
Outdated
refs: Int; | ||
} | ||
|
||
asm fun computeDataSize(cell: Cell, maxCells: Int): DataSize { CDATASIZE TRIPLE } |
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.
Would TRIPLE
work here? Structures are returned as tensors, right?
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.
TRIPLE
makes three elements into a tuple of length 3. I think it should work
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.
yes, but our structs are tensors, not tuples, right?
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.
structs are implemented as tuples in Tact (that's why we had problems with long ones in the first place, remember?)
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.
ah, I must have mistaken it with the unbundled approach in the storage
src/generator/writers/writeStdlib.ts
Outdated
ctx.asm("", "UBITSIZE"); | ||
}); | ||
|
||
ctx.fun(`__tact_sqrt`, () => { |
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.
how about we implement this in Tact?
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.
and what about other stdlib 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.
if a function can be implemented efficiently in Tact, we should implement it in Tact or open an issue and fix 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.
done
@anton-trunov @novusnota covered all new functions with tests! Found and fixed a few little mistakes in types (for |
@Gusarich Let's
|
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 should document the new functions in https://docs.tact-lang.org/ref/core-math and other relevant doc pages
Will do |
Co-authored-by: Novus Nota <68142933+novusnota@users.noreply.github.com>
Halfway there, will be finishing this soon. UPD: 2/3rds are done, got to complete the cell related functions |
… documented, except for `addressNone`
@anton-trunov have a look at the updated notes. there are few of them that I don't think we need to cover in Tact |
Looks good! Thanks for adding those |
Added documentation for all of the new functions from this PR, except for the |
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.
do we want to also add muldiv
?
Let's bring this up to date, resolve the review comments and merge |
Well, the only unresolved thing here besides merge conflics is the Therefore, I suggest @Gusarich you (or I) fix merge conflics, add the doc comments to stdlib based on the documentation I've added prior, and then the |
@novusnota @anton-trunov @Shvandre have a look please! |
<Badge text="Available since Tact 1.6" variant="tip" size="medium"/><p/> | ||
|
||
```tact | ||
extends fun depth(self: Cell): Int; |
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.
Maybe the argument should be optional? extends fun depth(self: Cell?)
. I haven't tested, but TVM.pdf (and TVM spec also) states
D765 — CDEPTH (c – x), returns the depth of Cell c. If c has no
references, then x = 0; otherwise x is one plus the maximum of depths
of cells referred to from c. If c is a Null instead of a Cell, returns zero.
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.
Good point, but calling null.depth()
in order to simply obtain 0 doesn't make much sense. And if the user has something like let x: Cell? = ...
at any point, I doubt calling x.depth()
will be an efficient way to determine if x
is null or not, since the !!
would still be needed for later uses that require Cell
and not Cell?
type.
But I might be totally wrong :)
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'd say it makes sense if your Cell? is fro, incoming message, and your logic is somehow based on that. I haven't seen those examples, but I would add that as there is no overhead, and internal TVM instruction provides support for it.
<Badge text="Available since Tact 1.6" variant="tip" size="medium"/><p/> | ||
|
||
```tact | ||
extends fun computeDataSize(self: Cell, maxCells: Int): DataSize; |
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.
Same comment here. What are the benefits of narrowing the application of a function? CDATAZISE
works fine with null
, and returns
@Gusarich is If not, we can remove it from the changelog, docs and sources for now. And then make a follow-up PR that'll re-introduce |
Issue
Closes #770
Towards #1029 (only
myCode()
)Checklist
docs/
and made the build locallyCurrent State
minmax
and noslice_bits_refs
because we don't have tensor types in Tact and it would be useless: Tensor types #1116set_data
andget_data
because Tact manages persistent storage by itselfbless
and other continuation functions because Tact manages control flow itself and for running some arbitrary code it's better to use RUNVM which doesn't require continuations parametersraw_reserve_extra
because it requires anuint32 -> varuint32
dictionary and we only supportvaruint16
for values currently:VarUInt32
,VarInt16
,VarInt32
serialization types #1117nan
andis_nan
because we don't support NaN in Tact. Do we really need them? @anton-trunovsub_rev
because it is useless in our case...MOD
instructions because they return two values as a tensor and it won't be very developer-friendly in Tact to make them return tuples in my opinion: Tensor types #1116