On Tue, 29 Nov 2022 16:46:43 GMT, Per Minborg <pminb...@openjdk.org> wrote:
> This PR proposes a variety of modernisations to the `jdk.sctp` module. > > During the fix of https://bugs.openjdk.org/browse/JDK-8296024, several > improvement areas were identified including: > > * Replacing duplicate code segments > * Making certain fields final > * Using enhanced switch > * Using records > * Fixing typos > * Marking fields participating in serialisation with `@Serial` > * Modernizing toString() implementations > * Using pattern matching > * Using diamond operators Various changes such as fixing spelling errors, addition of `@Serial`, removing redundant type arguments, using `instanceof` patterns, and using switch expressions seem fine to me. However, I'll defer to the judgment of the maintainers of this code. If a larger-scale refactoring is done to reduce the redundancy of UOE-throwing for the platforms where SCTP isn't supported, I'd recommend doing it in a separate bug/PR. src/jdk.sctp/unix/classes/sun/nio/ch/sctp/AssociationChange.java line 66: > 64: default -> throw new AssertionError( > 65: "Unknown Association Change Event type: " + intEvent); > 66: }; You might consider lining up the arrows of the non-default cases. The default case is quite different so I think it's ok to have it unaligned, in fact that's probably preferable. Similar alignment can be done in a few other places. src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpChannelImpl.java line 886: > 884: resultContainer.getSendFailed(), attachment); > 885: case SHUTDOWN -> absHandler.handleNotification( > 886: resultContainer.getShutdown(), attachment); Unfortunately since the code in each case is (probably) too long to fit on a line, lining up the arrows doesn't help. I guess what's here is ok. If I were maintaining this code, I'd try to make the names shorter in order for everything to fit on a single line... but that's probably out of scope. Also unfortunately, even though the code in each case has a very similar structure, there is redundancy across the classes that makes unifying the cases difficult. Specifically, resultContainer.type() returns a type, which is switched on to call different getter methods on resultContainer; the return value in turn is passed to different but corresponding overloads of absHandler.handleNotification. It seems to me that there ought to be a better way to structure this. Possibly pass the absHandler to the resultContainer and have it call the appropriate handleNotification() overload depending on what type it is? But that's also probably out of scope for this change. ------------- PR: https://git.openjdk.org/jdk/pull/11418
