-
Notifications
You must be signed in to change notification settings - Fork 77
Slang 1.1.0 #1123
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
base: v2
Are you sure you want to change the base?
Slang 1.1.0 #1123
Conversation
cfeff2b
to
66be80e
Compare
@@ -16,6 +16,8 @@ Nomic Foundation has put a lot of effort in providing a set of compiler APIs tha | |||
|
|||
Since v2.0.0 this package will ship with the Slang parser and this change must be implemented in existing configurations by replacing `parser: 'solidity-parse'` with `parser: 'slang-solidity'`. |
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 wonder if the parser names might confuse users here:
- For the new/default parser, WDYT of just using
slang
? since it is the project/NPM package/repository name.. - For the ANTLR parser, WDYT of using
deprecated-solidity-parser
instead ofsolidity-parse
? this matches the exact package name, and highlights that it is deprecated.
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.
My thought here is to keep the same historical name for the old parser in order for nothing to be break (while at least triggering a warning) and allow developers to slowly adopt the new parser.
I'm open to change this if @fvictorio thinks it's a better approach.
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.
as for the name slang
there is a possibility that version 2.1.0
could release with slang-yul
alongside slang-solidity
. if we feel this is a good feature.
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 see. I now remember the conversation about slang-yul
. Let's keep this as-is then. Thanks!
@@ -18,7 +18,7 @@ bytes32 private _CACHED_DOMAIN_SEPARATOR; | |||
/* solhint-enable var-name-mixedcase */ | |||
|
|||
|
|||
function() { | |||
function hello() { |
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.
Q: Is this due to the Slang upgrade?
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.
the solidity version needed to be 0.8.29
for the storage layout. but this version doesn't support nameless 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.
Thanks for explaining!
@@ -84,13 +84,15 @@ describe('inferLanguage', function () { | |||
} | |||
|
|||
test('should use the latest successful version if the source has no pragmas', function () { | |||
createParser(`pragma solidity 0.8.28;`, options); | |||
// This is to create in memory the latest parser and review the behaviour | |||
createParser(`pragma solidity ${latestSupportedVersion};`, options); |
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.
Not blocking of course, but just FYI: the newly shipped Languagefacts.inferSupportedVersions()
can now replace most of the custom implementation in createParser()
...
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 aware of this but was very busy moving out of my old house, travelling to Europe, and for the next few weeks I'll be AFK. So it didn't feel healthy to add more stress with an extra feature.
When I return to society I'll tackle this one.
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.
Of course. Thanks for all the great work so far!
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.
Left a couple of suggestions. Otherwise, LGTM!
cc @fvictorio
Adding support for storage layout.