Use enhanced switch#1505
Conversation
| canvas.setCursor(Cursor.getDefaultCursor()); | ||
| break; | ||
| case SELECTING: | ||
| canvas.setCursor(Cursor.getDefaultCursor()); |
There was a problem hiding this comment.
Indentation looks strange here, at least when it's in combination with the comment.
| srcAddress = UNSPECIFIED_ADDRESS; | ||
| break; | ||
| case SICSLOWPAN_IPHC_SAM_01: /* 64 bits */ | ||
| srcAddress = UNSPECIFIED_ADDRESS; |
There was a problem hiding this comment.
Same comment about strange indentation.
| case SICSLOWPAN_IPHC_DAM_01: /* 48 bits FFXX::00XX:XXXX:XXXX */ | ||
|
|
||
| } | ||
| case SICSLOWPAN_IPHC_DAM_01 -> { |
There was a problem hiding this comment.
Are you deliberately removing the comments after the colon? You removed some sizes in the switch dealing with SICSLOWPAN_IPHC_SAM_10 a page or two earlier.
If the comments are bad I think it's better to remove them, just double checking that it wasn't IntelliJ that wiped them for you.
| } | ||
| break; | ||
| case TRANSMISSION_FINISHED: { // Remove radio connection. | ||
| case TRANSMISSION_FINISHED -> { |
There was a problem hiding this comment.
Guessing the removed // Remove radio connection. wasn't intentional, so the earlier removed comments are probably also mistakes then.
| } | ||
| case BUFFER1_READ, BUFFER2_READ -> { | ||
| // Return bytes from the RAM buffer | ||
| buf_num = (state == BUFFER1_READ ? 1 : 2); |
There was a problem hiding this comment.
Github makes it hard to follow what happened here, but I'm guessing all lines are re-indented, and then Github decided to snap the diff view on a different place.
You might as well remove the parenthesis in the RHS here if the git blame gets trampled anyways.
| logw(WarningType.EXECUTION, "ERROR: Buffer Read past buffer size: " + bufferAddress); | ||
| } | ||
| case BUFFER1_WRITE, BUFFER2_WRITE -> { | ||
| buf_num = (state == BUFFER1_WRITE ? 1 : 2); |
There was a problem hiding this comment.
Same removing parenthesis comment here.
| log("RSSI is not valid"); | ||
| int RSSITIME = 380; | ||
| cpu.scheduleTimeEventMillis(rssiValid, RSSITIME / 1000.0); | ||
| // setState(CC1101RadioState.CC1101_STATE_RX); |
There was a problem hiding this comment.
Can remove a bunch of the leading spaces in the comment here.
| } | ||
| case CC1101_SWORRST -> log("CC1101_SWORRST not implemented"); | ||
| case CC1101_SNOP -> { | ||
| // log("CC1101_SNOP not implemented"); |
There was a problem hiding this comment.
Same about removing most of the leading spaces in this comment, and same for the one on line 302.
| //break; | ||
| case READ_RXFIFO: { | ||
| } | ||
| //break; |
There was a problem hiding this comment.
Can remove this line with the new switch.
| break; | ||
| } | ||
| } | ||
| // status |= STATUS_RSSI_VALID; |
There was a problem hiding this comment.
Can remove some of the leading spaces in the comment here too.
| updateGPIOConfig(); | ||
| } | ||
| } | ||
| // case REG_IOCFG1: |
There was a problem hiding this comment.
I think the comment should be updated with the -> { and ending } instead of break, so it's ready if someone uncomments it in the future.
| yield 0; | ||
| } | ||
| case RTCPS -> { | ||
| logNotImplemented("prescaling coutner"); |
There was a problem hiding this comment.
Since you're changing the line: coutner -> counter
| i = (index - TCCR0) / 2; | ||
| if (i >= noCompare) { | ||
| case TCCR0, TCCR1, TCCR2, TCCR3, TCCR4, TCCR5, TCCR6 -> { | ||
| int i; |
There was a problem hiding this comment.
Can declare this on the next line when defining it.
pjonsson
left a comment
pjonsson
left a comment
There was a problem hiding this comment.
Nothing stood out when reading this, but it was long with lots of changes. I assume you used IntelliJ or some other automatic tool to convert the switch-statements, so things should behave the same way as before the change.
I think the mentioned whitespace/comment stuff might be worth adjusting before merging, but go ahead and merge this after that.
|
Thanks for the feedback. Yes, I used IntelliJ to convert most of the switch statements (it failed on a few for some reason), but I need to take a closer look at the changes as I missed that some comments have been removed during the conversion. Unfortunately, there were a lot more code changes than I expected when I started to address the Error Prone issues, so an alternative could be to simply disable the relevant checks in Error Prone for now. |
Yeah, when I was working more actively on Cooja I didn't do the large enhanced switch changes for this reason. It's possible I missed some simple ones, but most of them triggered quite big diffs. Long-term, I think enhanced switches are the way forward, but I'm fine with disabling the check now as you suggest and then re-enabling it when we have migrated at our own pace. |
|
I made a PR #1515 that disables the check in ErrorProne for now. |
2 participants
Code cleanup to avoid warnings with Error Prone 2.38