Consume function braces at FunctionBody rule

[cgewecke]

This PR

• Relocates the opening and closing braces for function and modifier declarations to the function body rule, per the official grammar.
• Resolves #58 (See issue for further discussion)

A gist showing the current and proposed ASTs side by side can be found here. They differ in where they mark the start / end positions of the declarations and block statements.

NB: The code in this PR was authored by @duaraghav8 as part of solparse. He has given permission to integrate his bug-fixes into this repo so that solidity parser efforts can be consolidated in one place.

[cgewecke]

Yes, sounds good. I'll have that ready by tomorrow morning. Thanks for the review @federicobond.

I have a remedial git question: is it safe to squash the merge commit into the actual commit? Is there any way to keep the merge commit out of the PR without deleting my current fork and starting a fresh fork?

[federicobond]

You shouldn't introduce commits in your fork's master that are not in the upstream repo. To fix that:

git checkout master
git reset --hard "HEAD^"   # or when both forks diverge
git push -f

Make sure you configure a remote for the upstream repo like this:

git remote add upstream https://github.com/ConsenSys/solidity-parser.git

Then, you can do:

git pull upstream master

And bring all changes into your fork.

---

If you have a clean master synced with upstream, you should be able to rebase your PR branch by running:

git rebase -i master

This is the interactive rebase mode. You should see both the merge commit and the Consume function... commit. Remove the first line, save and close the file. Git should do the rest, but it may ask you to fix some conflicts. If that's the case, fix them, add the files and run git rebase --continue.

When you are done, git push -f to the PR branch to update your PR. After refreshing the page you should see a single commit that merges cleanly against latest master.

[cgewecke]

@federicobond. Thank you, that is incredibly helpful.

[cgewecke]

@federicobond Revised with your suggestions. Have update AST gist to show output for some simple cases.

[federicobond]

One thing I note there is that the updated AST contains no init value for x = 5;

Might have something to do with one of the last PRs.

El El lun, 23 de ene. de 2017 a las 23:31, <-c-g-> notifications@github.com escribió:

@federicobond https://github.com/federicobond Revised with your suggestions. Have update AST gist https://gist.github.com/cgewecke/9faa561c644eaa9d2d455c1b1bb96198 to show output for some simple cases.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ConsenSys/solidity-parser/pull/61#issuecomment-274683682, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIcunccUFWEbyuGkCG5HJc4Yt3XMcpvks5rVWJwgaJpZM4LqqI9 .

[cgewecke]

No, i'm sorry the code in the example just doesn't match the output. It's a typo. I'll change it right now.

[cgewecke]

Would be really nice to have tests that check the ASTs

[federicobond]

OK, good to know the AST is not broken. Indeed, we could snapshot the AST for the test solidity file we have right now, so each change in the AST is version-controlled.

[federicobond]

Another thing that is clear for me now is that there is no need to have both FunctionBody and Block, and we can probably also consolidate StatementList / Statements into a single rule too.

[cgewecke]

Yes there's some redundancy there. This is small but I think I failed to take out some extra space symbols in the modifier declaration rule where functionBody was changed.

[federicobond]

Should I merge this? BTW, thanks for all your contributions, much appreciated!

[cgewecke]

Yes merge, sorry. And I guess clean up in the next sweep across this stuff. Thanks federico!

[cgewecke]

@federicobond Are you holding off on doing a parser rebuild? None was included in this set of changes and was hoping to do a little work on Solium this weekend with the updates.

[federicobond]

Sorry, my machine broke down last week and I haven't set up my development environment for solidity-parser since then. Would you mind updating the parser in a PR for me?

El El jue, 26 de ene. de 2017 a las 13:03, <-c-g-> notifications@github.com escribió:

@federicobond https://github.com/federicobond Are you holding off on doing a parser rebuild? None was included in this set of changes and was hoping to do a little work on Solium this weekend with the updates.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ConsenSys/solidity-parser/pull/61#issuecomment-275427629, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIcujU72pyPiJzgKhsdNvPPd0XoUb-yks5rWMPMgaJpZM4LqqI9 .

[cgewecke]

Yes, I'll do it right now.