remove implicit fallthroughs (#1194)#1196
Conversation
| switch (conditions) { | ||
| case 0: if (!oneIn(4)) continue; | ||
| case 1: if (!oneIn(2)) continue; | ||
| case 0: if (!oneIn(4)) continue; break; |
There was a problem hiding this comment.
I think having it all on one line like this makes this difficult to read (i.e. it's not obvious that the continue is conditional but the break is not). Could we do
case 0:
if (!oneIn(4)) continue;
break;
instead?
| case 0: if (!oneIn(4)) continue; | ||
| case 1: if (!oneIn(2)) continue; | ||
| default: if (oneIn(conditions + 1)) continue; | ||
| case 0: |
There was a problem hiding this comment.
since the body isn't on the same line, please add {,} for each (see other switches for the convention)
|
Perfect, thanks! Just waiting for CI to finish before merging. |
|
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 |
|
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 :) |
|
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 Without |
|
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 |
|
Yeah, I agree. Let's just run |
|
I ran 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? |
|
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. |
|
Yeah, perfect. Thanks again! |
Fixes two implicit fall through warnings which caused building to fail.
Closes #1194