On Wed, 29 Sep 2021 22:03:00 GMT, Joe Darcy <da...@openjdk.org> wrote:
>> Augmentations to javac's Xlint:serial checking are out for review (#5709) >> and various javac and javadoc implementation libraries would need some >> changes to pass under the expanded checks. >> >> The changes are to suppress warnings where non-transient fields in >> serializable types are not declared with a type statically known to be >> serializable. That isn't necessarily a correctness issues, but it does merit >> further scrutiny. >> >> I'll run a script to update the copyright year before pushing. >> >> Sending this change out for review separately in an effort to de-bulk the >> review needed for the new checks themselves. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. There are 3, maybe just 2, groups of files in this review. `sjavac` is an internal utility that ought not to be in the `src/` tree, so the changes there don't matter. The various `Result` classes and the javadoc exceptions are all "internal" exceptions used for internal control flow, and not intended to be seen by API clients. As @pavelrappo notes, we have plans to make the `Result` classes go away by rewriting the relevant scanners to make them unnecessary. That leaves the javadoc exceptions. The commented annotations have a slight code-smell about them, in the sense of brushing the warning under the carpet, so to speak. It would be better if there was a better way to remove the cause of the warning, rather than just hiding the warning itself. But, that's not easy, and the original sin was making `Throwable` (and hence all subtypes) implement `Serializable` in the first place. But, that's the serialization we have and we have to deal with it. As a final note, I never did like the `Runnable` in the last exception, and seeing it again may be a good reason to try and get rid of it, like those `Result` classes mentioned earlier. I also note that `DocPath` _could_ be made `Serializable` but `DocFile` cannot reasonably be made serializable (incompatible API change to `Location`) and even thinking about it seems like a case of the tail wagging the elephant! So, with some disappointment, I accept that the changes you propose are the least bad of the possible solutions, at least for now, and so I approve the review. ------------- Marked as reviewed by jjg (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5728
