On Fri, 17 May 2024 15:02:14 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> Kevin Rushforth has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 20 additional >> commits since the last revision: >> >> - Wait for re-review if you've pushed a change addressing a substantive >> comment >> - Typo + remove sentence that invites reformatting PRs >> - Wording changes, added example of API addition >> - Formatting >> - Add item about checking the target branch >> - Remove trailing period from previous >> - Minor changes regarding syncing from upstream master >> - Clarify that GHA tests might fail for a reason unrelated to the PR >> - Fix typo >> >> "but It" --> "but it" >> - Remove bullet about checking the commit message (Skara already checks) >> - ... and 10 more: https://git.openjdk.org/jfx/compare/1a4c0fd4...9cf7f920 > > README-code-reviews.md line 69: > >> 67: * Carefully consider the risk of regression >> 68: * Carefully consider any compatibility concerns >> 69: * Check whether it adds any new public or protected API, even implicitly >> (such as a public method that overrides a protected method, or a class that >> is moved from a non-exported to an exported package); if it does, indicate >> that it needs a CSR > > I think that if it looks like an oversight (the author forgot about the > default constructor), it's better to indicate that than to indicate that the > PR needs a CSR. Maybe something like: > "if it does and it doesn't look like an oversight, indicate that it needs a > CSR" We have by now cleaned up our public API to avoid classes with an implicit no-arg constructor, so the only way this situation could arise in the future is if someone adds a new public class, which needs a CSR anyway. I guess it could also happen if someone proposed to delete the no-arg constructor, but we don't allow deleting of non-terminally-deprecated API anyway. Maybe it's worth changing "adds any new" to "adds, removes, or modifies any"? Somewhat related, these guidelines don't address what to look for when reviewing new API; perhaps a follow-on issue. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605797980
