-
Notifications
You must be signed in to change notification settings - Fork 78
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
Add builtins substr and concat for ByStr and to_bystrx to convert to fixed length byte strings #906
Conversation
Co-authored-by: Anton Trunov <anton@zilliqa.com>
src/base/Syntax.ml
Outdated
@@ -178,7 +180,15 @@ let parse_builtin s loc = | |||
| "to_uint64" -> Builtin_to_uint64 | |||
| "to_uint128" -> Builtin_to_uint128 | |||
| "to_nat" -> Builtin_to_nat | |||
| _ -> raise (SyntaxError (sprintf "\"%s\" is not a builtin" s, loc)) | |||
| _ -> ( | |||
let err = SyntaxError (sprintf "\"%s\" is not a builtin" s, loc) in |
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.
Btw, if we care for speed, we should not create an error prematurely, only when it's not really needed.
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 doesn't seem like that expensive an object to create right? Compared to regex for example. I put it at the beginning because it has two uses.
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 think I'll remove that comment as the code we have now seems short enough (compared to longer thing I had before)
Co-authored-by: Anton Trunov <anton@zilliqa.com>
LGTM! Just one tiny nit: can we also have a test case with failing parsing of the new syntactic element? (i.e. |
Done |
I will not squash-merge this PR as I intend to continue working on this branch for further for polynetwork support.Since I now have a format commit too, I'll just squash merge it and deal with the merge back topolynetwork
branch later.