-
Notifications
You must be signed in to change notification settings - Fork 7
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
LSP: Simplify parser and parse tree for LSP #214
Conversation
40c7075
to
05d991b
Compare
@@ -81,7 +81,7 @@ not: 139 | |||
not?: 139 | |||
or?: 139 | |||
pact-id: 60000271 | |||
pairing-check: 67003096 | |||
pairing-check: 67003097 |
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'm wondering what has caused the pairing-check gas increase?
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 ran this code using master and adding (env-milligas)
to the repl file. Loading it, here's the outcome:
Master
pact>(load "pact-tests/gas-goldens/pairing-check.repl" true)
"Loading pact-tests/gas-goldens/pairing-check.repl..."
"Set gas model to table-based cost model"
"Set gas limit to 150000"
Loaded module verifier, hash ArzjsOeeMDrskpTEaedsj0sfdXzv4RA3GP5oOTMnTTw
60012686
"Expect: success Pairing function check on test proof"
67003096
This PR:
pact>(load "pact-tests/gas-goldens/pairing-check.repl" true)
"Loading pact-tests/gas-goldens/pairing-check.repl..."
"Set gas model to table-based cost model"
"Set gas limit to 150000"
Loaded module verifier, hash xpLf2PRtsXDD940UWi0hkWWrcNb3G-teyE2gfSPQP-k
60012687
"Expect: success Pairing function check on test proof"
67003097
Notice the 1 milligas change. Likely changing the encoding changed the sizeOf
which made us charge more gas, since we go through 1 more constructor for certain things.
It's just the module encoding changed. May have gotten microscopically bigger, but in return we dropped several constructors, which is a net win.
@@ -92,7 +92,7 @@ | |||
|
|||
;; check events | |||
(expect "step 0 events" | |||
[{"module-hash": "oKFbzkuEYAFhP-S2mW7hRvRJUPJf2FvMFy1CpxhGs4o" | |||
[{"module-hash": "rmN99MpBmJbapgVRV3GjII6I4UUkl8k5pBF7-k92Lt8" |
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 module-hash based on stable encoding? It shouldn't change?
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 not based on stable encoding. It should change. It's based on our CBOR encoding.
05d991b
to
737bc4e
Compare
This PR is a prelude that's necessary for further LSP work. It essentially simplifies the parser and moves a lot of the work that was previously done in the tokenizer + parser into the desugarer.
The parse tree is thus simplified significantly and far more independent of what we need for execution, which will help our LSP provide more accurate information on hover (the LSP analyzer is coming in a subsequent PR)
It also improves repl docs. That is, pre this pr:
Post:
PR checklist:
Additionally, please justify why you should or should not do the following: