Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

[fix] Incompatible match arms in parity-clib#9705

Merged
dvdplm merged 1 commit into
masterfrom
dp/fix/clib-incompatible-match-arms
Oct 5, 2018
Merged

[fix] Incompatible match arms in parity-clib#9705
dvdplm merged 1 commit into
masterfrom
dp/fix/clib-incompatible-match-arms

Conversation

@dvdplm
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm commented Oct 5, 2018

This error only shows up when building with --all-features so the jni code is built.

@dvdplm dvdplm requested a review from tomaka October 5, 2018 07:20
@dvdplm dvdplm self-assigned this Oct 5, 2018
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. M5-binaries 📦 External binaries (ethkey, ethstore, ethvm, etc.) labels Oct 5, 2018
niklasad1
niklasad1 reviewed Oct 5, 2018
View reviewed changes
Copy link
Copy Markdown
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Basically, this changes the behavior if we get a "malformed" CLI argument we return an error (returns early) instead of calling parity?

I have "no real" opinion whether we should return early with an error or run parity with the remaining arguments (possibly empty) and throw an exception that the user has to deal with!

@dvdplm
Copy link
Copy Markdown
Collaborator Author

dvdplm commented Oct 5, 2018

Basically, this changes the behavior if we get a "malformed" CLI argument we return an error (returns early) instead of calling parity?

Well, hm, without this the code doesn't compile (match arms differ in return type: the first is () and the second was jlong.
I suspect this was a just a typo inspired by the match just below it that also raises a java exception and returns 0. TBH I don't know what 0 means in this context, it can't really be the shell "all good" retval right?

@niklasad1
Copy link
Copy Markdown
Collaborator

niklasad1 commented Oct 5, 2018 •
edited
Loading

aha, 0 is definitely an error because on success this function returns the address to the out pointer but @tomaka has to confirm this to be sure :P

@tomaka
Copy link
Copy Markdown
Contributor

tomaka commented Oct 5, 2018

Throwing an exception short-circuits everything, so the actual return value doesn't matter.

@niklasad1 niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 5, 2018
@dvdplm dvdplm merged commit 4f278ba into master Oct 5, 2018
@sorpaas sorpaas deleted the dp/fix/clib-incompatible-match-arms branch October 5, 2018 10:44
dvdplm added a commit that referenced this pull request Oct 9, 2018
@dvdplm
Merge remote-tracking branch 'origin/master' into dp/chore/update-com…
1dae486 
…mon-deps

* origin/master:
  fix (light/provider) : Make `read_only executions` read-only (#9591)
  ethcore: fix detection of major import (#9552)
  return 0 on error (#9705)
  ethcore: delay ropsten hardfork (#9704)
  make instantSeal engine backwards compatible, closes #9696 (#9700)
  Implement CREATE2 gas changes and fix some potential overflowing (#9694)
  Don't hash the init_code of CREATE. (#9688)
  ethcore: minor optimization of modexp by using LR exponentiation (#9697)
  removed redundant clone before each block import (#9683)
@5chdn 5chdn added this to the 2.2 milestone Oct 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@tomaka tomaka tomaka approved these changes

@niklasad1 niklasad1 niklasad1 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dvdplm dvdplm

Labels

A8-looksgood 🦄 Pull request is reviewed well. M5-binaries 📦 External binaries (ethkey, ethstore, ethvm, etc.)

Projects

None yet

Milestone

2.2

Development

Successfully merging this pull request may close these issues.

4 participants

@dvdplm @niklasad1 @tomaka @5chdn