-
Notifications
You must be signed in to change notification settings - Fork 53
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
Draft fix for #26 #44
Conversation
This is a step in the good direction, but I would prefer to separate state variable specifiers from local ones, and also replace all those boolean keys in AST nodes with Local variables, as far as I understand, only accept a |
@federicobond I'd like to move this PR forward but I could use a little design help if that's ok. In your view
|
Thank you for your time @cgewecke, I am happy to help. As a general rule, whenever in doubt, check the result of the official solidity compiler AST or the Regarding your questions:
For state variables, I believe you can do |
Great, thanks so much @federicobond. Will revise as you suggest. |
@federicobond Sorry for letting this languish for weeks. One reason has been that while I agree the parser should have the precision of the grammar, the proposed changes to I'm wondering if you would consider the following:
I realize this is some insane hopscotching around but it might help knit some of the core utilities to a common parser base and prevent further drift. I'd be very happy to help with the above. I also understand if it's too complicated or should be done in a different order . . . |
@cgewecke that sounds great. I thought about asking you for an update earlier today, but it realized it was too early in the year to bother volunteers :) Your plan sounds good, and you have my full support for that. As long as every change is documented in the changelog, the worst that can happen is that some downstream project must fix their declared dependency versions until they migrate. cc/ @tcoulter |
For reference, my Solidity ANTLR4 grammar is much more conformant with the reference implementation at this point and uncovered several errors in the official grammar as published in the docs. You can use it as a reference when making changes. |
@federicobond Awesome. Thanks for the grammar link. |
@cgewecke can you rebase and remove the built parser so it merges cleanly? |
While you are there, please rewrite your commit as "Allow declarative..." |
@federicobond Would it be ok if I close this, refork and resubmit. The odds of me rebasing correctly are 0%. Also should I include a build or not? |
@federicobond On second thought I'm actually in favor of closing this period and just not merging. You've proposed a much better set of changes that should be their own thing. I apologize for this whole mess. |
I think it's best to not include a build. Otherwise, any two simultaneous pull requests will most likely generate a merge conflict. |
OK, let me know if you need any help. I am a bit overworked at the moment but I would love to move this forward by next week. |
Great - I'll write up your idea over the weekend and I think there will be some small PRs coming out of an analysis of the difference between solparse and solidity parser early next week too. |
This PR rewrites the declarative expression rule to allow descriptors like
constant
andinternal
to appear in any order.It
DeclarativeExpression
's optional descriptor series with greedy collectionAlso adds example to
doc_examples
and has rebuild.