Skip to content

Update syntax for Solidity 0.8.8 #58

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

Merged
merged 2 commits into from
Sep 27, 2021
Merged

Update syntax for Solidity 0.8.8 #58

merged 2 commits into from
Sep 27, 2021

Conversation

haltman-at
Copy link
Collaborator

Syntax updates for Solidity 0.8.8:

  1. I reclassified type as a keyword rather than a builtin, now that it can be used for custom type declarations.

  2. I highlighed wrap and unwrap as builtins when used as a member of something else. I'm a little uncertain whether this is a good idea; it's possible that wrap and unwrap functions are sufficiently common that highlighting this would be annoying, and it would be better to just omit highlighting for these. So, uh, please let me know what you think of this. (@gnidan, I can't add you as a reviewer, but do you have an opinion on this?)

@cds-amal
Copy link
Collaborator

  1. I reclassified type as a keyword rather than a builtin, now that it can be used for custom type declarations.

This seems reasonable.

  1. I highlighed wrap and unwrap as builtins when used as a member of something else. I'm a little uncertain whether this is a good idea; it's possible that wrap and unwrap functions are sufficiently common that highlighting this would be annoying, and it would be better to just omit highlighting for these. So, uh, please let me know what you think of this. (@gnidan, I can't add you as a reviewer, but do you have an opinion on this?)

I think if they are sufficiently common it is a good thing to draw attention to, especially as it is a new feature.

Copy link
Collaborator

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

'name creationCode runtimeCode interfaceId min max'
'send transfer call callcode delegatecall staticcall ' +
'balance code codehash ' +
'wrap unwrap ' +
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was pre-existing but it would be nice to have comments that explain where these groupings come from: Address / user types, type information etc

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I could add that!

@haltman-at
Copy link
Collaborator Author

I think if they are sufficiently common it is a good thing to draw attention to, especially as it is a new feature.

I'm not sure I understand what you're saying here. I'm saying that if x.wrap and x.unwrap are sufficiently common as existing things, it might be obnoxious to highlight. Ideally not every instance of x.wrap or x.unwrap would be highlighted, only those cases where x is a UDVT. But that's not something we can feasibly do with HLJS; it's either highlight all such cases or none of them. So the question is, would making it highlighted in all cases -- to get it highlighted when x is a UDVT -- be too obnoxious due to existing (and non-conflicting) usage of x.wrap or x.unwrap in other cases?

I don't think I understand what your answer to that question is.

@cds-amal
Copy link
Collaborator

But that's not something we can feasibly do with HLJS; it's either highlight all such cases or none of them.

Oh, right! There's not enough information to disambiguate. I misunderstood your 2nd point. However I still think that if this is a new feature we should draw attention to it and wait for feedback that it is noisy.

This Github search suggests wrap is not so popular in GH public repos

@haltman-at
Copy link
Collaborator Author

OK, cool, I'll go ahead with that then. I'll go back and add the comments you suggested and then merge / release this.

I asked about wrap/unwrap because earlier I had contemplated making value a builtin (#24) and @frangio was like no that'd be way too noisy. (We ended up coming up with a special-case hack to handle it.) I knew that like wrapped ether was a thing so I wasn't sure if that would be a common existing usage that would cause noise? Wrap/unwrap also just seemed like it might be a common pair in general. But sounds like it isn't so there's no problem!

@haltman-at haltman-at merged commit 4977f17 into master Sep 27, 2021
@haltman-at haltman-at deleted the updates-088 branch September 27, 2021 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants