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

remove implicit fallthroughs (#1194) #1196

Merged
merged 4 commits into from Sep 20, 2017

Conversation

Projects
None yet
3 participants
@z2oh
Contributor

z2oh commented Sep 19, 2017

Fixes two implicit fall through warnings which caused building to fail.

Closes #1194

Show outdated Hide outdated src/tools/translate-to-fuzz.h Outdated
@dschuff

LGTM, thanks!

Show outdated Hide outdated src/tools/translate-to-fuzz.h Outdated
@kripken

This comment has been minimized.

Show comment
Hide comment
@kripken

kripken Sep 19, 2017

Member

Perfect, thanks! Just waiting for CI to finish before merging.

Member

kripken commented Sep 19, 2017

Perfect, thanks! Just waiting for CI to finish before merging.

@dschuff

This comment has been minimized.

Show comment
Hide comment
@dschuff

dschuff Sep 19, 2017

Member

lol @z2oh sorry you hit a perfect storm of conflicting reviewers. This isn't the first time @kripken and I have clashed on exactly this bit of style :D

Member

dschuff commented Sep 19, 2017

lol @z2oh sorry you hit a perfect storm of conflicting reviewers. This isn't the first time @kripken and I have clashed on exactly this bit of style :D

@kripken

This comment has been minimized.

Show comment
Hide comment
@kripken

kripken Sep 19, 2017

Member

Oh sorry, I didn't notice there were previous comments - they're hidden in the diff view.

@dschuff, did we ever come to a consensus on that? Looking in the code most have the {,} but I don't remember details of discussion on the topic.

Member

kripken commented Sep 19, 2017

Oh sorry, I didn't notice there were previous comments - they're hidden in the diff view.

@dschuff, did we ever come to a consensus on that? Looking in the code most have the {,} but I don't remember details of discussion on the topic.

@dschuff

This comment has been minimized.

Show comment
Hide comment
@dschuff

dschuff Sep 19, 2017

Member

I think you won mostly just by virtue of having written a lot of code in that style before I did, so I didn't want to change it all :)

Member

dschuff commented Sep 19, 2017

I think you won mostly just by virtue of having written a lot of code in that style before I did, so I didn't want to change it all :)

@kripken

This comment has been minimized.

Show comment
Hide comment
@kripken

kripken Sep 19, 2017

Member

Heh, ok :) Anyhow, I'm not super-invested in this - if most people prefer another convention let's switch (separate from this PR of course).

But just for the record (since I'm not sure where else this is documented), my logic is that this is similar to the case of

if (..) {
  ..
} else {
  ..
}

Without {,} there is a risk of getting things wrong, and switch cases also denote separate code paths like if arms.

Member

kripken commented Sep 19, 2017

Heh, ok :) Anyhow, I'm not super-invested in this - if most people prefer another convention let's switch (separate from this PR of course).

But just for the record (since I'm not sure where else this is documented), my logic is that this is similar to the case of

if (..) {
  ..
} else {
  ..
}

Without {,} there is a risk of getting things wrong, and switch cases also denote separate code paths like if arms.

@dschuff

This comment has been minimized.

Show comment
Hide comment
@dschuff

dschuff Sep 19, 2017

Member

Oh hey, the translate-to-fuzz test is failing becuase... yeah this actually does change the behavior. Before, we had multiple rolls of the RNG to continue and now it's just one. Since that was probably the intended behavior, should we just auto-update the tests?

Member

dschuff commented Sep 19, 2017

Oh hey, the translate-to-fuzz test is failing becuase... yeah this actually does change the behavior. Before, we had multiple rolls of the RNG to continue and now it's just one. Since that was probably the intended behavior, should we just auto-update the tests?

@kripken

This comment has been minimized.

Show comment
Hide comment
@kripken

kripken Sep 20, 2017

Member

Yeah, I agree. Let's just run ./auto_update_tests.py and add that change here.

Member

kripken commented Sep 20, 2017

Yeah, I agree. Let's just run ./auto_update_tests.py and add that change here.

@z2oh

This comment has been minimized.

Show comment
Hide comment
@z2oh

z2oh Sep 20, 2017

Contributor

I ran ./auto_update_tests.py and a subsequent run of ./check.py passed on my system. Here is the output of git diff: https://hastebin.com/zayuraniho .

I know very little about the testing process of this project, so I can't really say if this is correct or not. Should I commit this change?

Contributor

z2oh commented Sep 20, 2017

I ran ./auto_update_tests.py and a subsequent run of ./check.py passed on my system. Here is the output of git diff: https://hastebin.com/zayuraniho .

I know very little about the testing process of this project, so I can't really say if this is correct or not. Should I commit this change?

@dschuff

This comment has been minimized.

Show comment
Hide comment
@dschuff

dschuff Sep 20, 2017

Member

That looks about right to me. It's just the one test that's affected, and it will be affected in a random way, so it's probably correct. I'd say go ahead and commit it, and if @kripken agrees with my assessment we'll merge.

Member

dschuff commented Sep 20, 2017

That looks about right to me. It's just the one test that's affected, and it will be affected in a random way, so it's probably correct. I'd say go ahead and commit it, and if @kripken agrees with my assessment we'll merge.

@kripken

This comment has been minimized.

Show comment
Hide comment
@kripken

kripken Sep 20, 2017

Member

Yeah, perfect. Thanks again!

Member

kripken commented Sep 20, 2017

Yeah, perfect. Thanks again!

@kripken kripken merged commit b29158d into WebAssembly:master Sep 20, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment