Code Review Tips and Recommendations

Stuart W. Marks

(I presented this at the OpenJDK Committers' Workshop on 2 Aug 2018. These are really rough notes, and they might not be intelligible if you weren't at the presentation. But it should serve as a starting point for discussion.)

For Developers

• Post review to appropriate mailing list
  • RFR = "request for review"
  • loose convention is xs/s/m/l/xl to indicate size
  • indicate which release is being targeted
  • provide links to bug and to webrev
  • in JIRA, add review thread link using More > Link, then choose Web Link, then entire URL and link title
• Make sure changeset comment is available as part of review
• Changeset comment should match bug summary; adjust either as necessary
• Include patch inline if it's trivial and if the patch fits onto one page (maybe 2-3 hunks), otherwise use webrev
• Use rsync to get webrev files onto cr.openjdk.java.net
  • An alternative is scp -r but rsync seems more flexible; in particular, it provides the ability to remove remote files
• Add link to email review thread as a comment in the bug report
• Multiple webrevs
  • For significant revisions, post another webrev and link to it
  • Avoid updating a webrev in-place, as it invalidates earlier email comments
  • Only update a webrev in-place if changes are trivial (spelling errors, spacing)
  • The last webrev posted needn't match the actual changeset, if the differences are trivial
  • Generally try to leave webrevs in the CR area; no need to "clean up" as this will break old links in email and bugs
  • summarize substantial email comments in a comment in the bug report

For Reviewers: Process, Mechanical, and Editorial

• Keep editorial comments (spelling, formatting, spacing, wording, code conventions) separate from substantive comments
  • Avoid editorial comments in preliminary reviews, when design isn't settled yet
  • Editorial stuff is important if review is in final stages
• Are language constructs and APIs used appropriately?
  • generics
  • try-with-resources
  • lambdas, collections, streams
  • concurrency & memory model
• Are files being added in the right locations? (especially tests)
  • Name tests semantically, not after bug ID
• Do files have the proper copyright notice?
  • product code generally has GPL with classpath exception
  • test code generally has GPL without classpath exception
  • update copyright years as necessary
• Ensure that the proper JIRA issue id is in changeset comment
  • JIRA issue should be: bug, enhancement, or subtask
  • JIRA issue should not be: backport, CSR, or JEP
• Is the fix being pushed to proper repo? (right release, right project)
  • the webrev often lists the destination repo at the top
  • (but sometimes it doesn't, if the author has used webrev -N)
  • generally one can infer the destination repo from the release or project named in the subject line (e.g., JDK 11, JDK 12, Jigsaw, Amber, Valhalla, etc.) and from the mailing list on which the review occurs, but it's still possible for a changeset to be pushed to the wrong repo accidentally
• Are the right mailing lists included in the review?
  • in particular, build changes should be sent to build-dev
• Does the fix meet current release criteria? Have necessary approvals been obtained? See JEP 3.
• Is a regression test necessary? If not, make sure bug has noreg-xxx label
  • See OpenJDK Change Planning, section 6 for noreg-xxx labels
• Is a CSR necessary? (for API changes, or significant behavior changes)
• Is a release note necessary?
• Make sure Form 27B/6 has been filed.
• Is it worth asking about code coverage?
• Is it worth asking about performance?
• Has an external contributor signed the OCA?
  • employees of comtributing companies are covered under corporate contributor agreeement
  • individual signers can be found on the OCA page
  • if an individual hasn't signed, ask them to follow instructions on the OCA page
  • ask Dalibor Topic dalibor.topic@oracle.com if there are any questions

For Reviewers: Semantics

• Does the change itself make sense? Does the new state of the code make sense after change has been applied?
  • These are different points of view. Sometimes in the context of the change, replacing X with Y makes sense. But reading the new version in isolation, question may arise: why is Y here?
• Does the change comport with the specification?
• Failure idempotency
• Do any abstractions need to be adjusted as a result of the change?
  • If redundancy is introduced, maybe it should be unified
  • If dead code is removed, maybe this is an opportunity for simplification
• Is code outside the changeset that are affected by the change?
  • Do similar changes need to be applied elsewhere?
• Are the names used any good?
  • variables
  • parameters
  • methods
  • classes
  • packages (e.g., sun.* vs jdk.internal.*)
  • properties (e.g., java.* vs jdk.* vs sun.*)
• Do constructs have the right access levels? (public, protected, package access, private)
  • stuff visible in specifications must be public or protected
  • implementation things are mostly package access, and rarely private
  • private used to cause introduction of bridge methods (does Nestmates fix this?)
• Consider all aspects of compatibility
  • source
  • binary
  • behavioral
  • serialization
  • wire format or file format
• Does the change introduce any new namespaces?
  • Where are names defined?
  • Is it controlled or extensible? If extensible, is there a registry or convention?
  • Example: Java package namespace, registry piggybacks on DNS
  • Example: @SuppressWarnings("xyzzy")?? no namespace control
• "Security issues are not discussed in this memo." (RFC 2316)
  • Information leakage
  • Encapsulation (defensive copies)
  • TOCTOU (= time-of-check vs time-of-use)
• Are the tests reasonable and well-behaved?
  • Much more could be written about this, but here are some highlights of issues
  • run quickly
  • be reliable
• avoid race conditions
• take care that all possible randomized inputs will work
• avoid intermittent failures
  • a regression test should test the bug that was fixed; in particular, it should always fail in the absence of the fix and always pass with the fix
  • use proper keywords (e.g., randomness)
  • clean up all resources (files, sockets, subprocesses)
  • consider TestNG for fine-grained unit tests, jtreg for system tests (requiring JVM options, properties, multiple JVMs, etc.)

Code Review Scenario

Consider the following code review request.

[12] RFR(xs): 8901234: Foo.getBar() throws NullPointerException

  public class Foo {
       static Bar bar;
       /* constructors, setters, and other initialization code for bar */
       static Bar getBar() {
 -         return bar.makeCopy()
 +         return (bar == null) ? null : bar.makeCopy();
       }
   }

One possible review:

"Looks good to me."

Potential review questions:

• This certainly avoids NPE here, but can it cause the caller to NPE?
• Why was bar null here?
• Should bar have been initialized to be non-null?
• If it was, was it later changed back to null?
• Should there be an invariant where bar is always non-null?
• Should bar be final, in addition to always being initialized non-null?
• Is the caller prepared to receive a null return?
• Should Optional be returned instead?
• Are there other fields in this class that might also be null?