Code commenting guidelines

Comments are there to help the author of the code as well as current and future co-workers.

1 Motivation and strategy
2 General guidelines
3 Updating comments over time
4 Engineering standards (checked during reviews)
5 Examples

Motivation and strategy

Comments help yourself as much as your co-workers to remember the important bits: code you wrote 6 months ago might as well have been written by someone else — you’ll thank yourself for the comments you are leaving for your future self.
Comments make a code base more approachable for new engineers.
Especially package-level or top-of-file comments that explain how pieces fit together.

General guidelines

If you had to think about the design of your code, explain your design in prose in a comment no later than when you submit the code for review.
Write as if the audience was yourself prior to you writing the code.
When exploring / analyzing existing code, if you notice that you need to think hard to understand it, then make a PR to add comments with your new understanding. Also fix typos and grammar mistakes that impair the reading, as you find them.
Do so even if it's not your own code.
When there are questions during a code review, add comments to explain the outcome of the review:
if the reviewer had questions, the next reader will too.

Updating comments over time

When you discover something valuable that should be explained in a comment, but is not yet explained, consider adding/extending a comment with your new knowledge. The next reader will thank you.
If you find a mistake in a comment, it can be a good use of your time to fix it.
- If a comment is factually incorrect, then you should consider it as an outright bug and either attempt to fix it immediately or file an issue.
- If it contains a grammatical or spelling error that makes the reading particularly difficult, it would be valuable to the team and your future self to fix it too.
Beware of “cosmetic” improvements, e.g. changing one word for another, or fixing “simple” typos that don’t hurt reading. The time spent on creating a commit and a PR and getting it reviewed might not be worth the marginal value of the improvement, unless you can combine the improvement with a larger project.
Also consider the marginal impact of aesthetic changes on automation and the output of git blame, as discussed here.

Engineering standards (checked during reviews)

Do’s:

Use full sentences. Capital letter at begin, period at the end, with at least a subject and a verb.
In a struct, API or protobuf definition, there should be more comments than code.
- The comments explain the purpose of the fields, as well as their lifecycle: what code initializes them, what code accesses them and why, when fields become obsolete, etc.
- This rule becomes a “must” if the struct/API/protobuf is part of a public/exported API.
Throughout a function body, use comments to separate and summarize different “phases” of the processing in the control flow.
- Write these comments such that they would be good explanatory comments if the corresponding part of the function body was extracted into a separate function.
Make an honest attempt at keeping your comments grammatical.
Conversely, assume any reviewer suggestions about grammar is done on a neutral and non-accusatory tone. As a reviewer, prefix grammar-related comments as “nit:”.
During reviews, point out when adding a (missing) comment would have helped your understanding.
Give praise for well-written comments.

Don’t:

Write comments that read the code underneath “out loud”, for example:
- “this line increments the variable x” for x++.
- “now we iterate over all the strings” for for i := range strs
When reviewing a PR, put too much emphasis on comment grammar.
Grammar remains prefixed by “nits:” in reviews.

Examples

Top-level design comments

Top-level design comments explain the concepts/abstractions used throughout a package, shows at a high level how the pieces fit together, and connects the code to its use cases / “how it is used from outside”.

At the top of concurrency/concurrency_control.go
[source link]
At the top of authentication_oidc.go
[source link]
In sql/opt/doc.go
[source link]
In storage/doc.go
[source link]

Comments on protobuf messages

From roachpb/metadata.proto: a large comment in front of an enum type provides a birds-eye view of how the pieces fit together; each individual value is commented in how it is used in the remainder of the code.

// ReplicaType identifies which raft activities a replica participates in. In
// normal operation, VOTER_FULL and LEARNER are the only used states. However,
// atomic replication changes require a transition through a "joint config"; in
// this joint config, the VOTER_DEMOTING and VOTER_INCOMING types are used as
// well to denote voters which are being downgraded to learners and newly added
// by the change, respectively. A demoting voter is turning into a learner,
// which we prefer over a direct removal, which was used prior to v20.1 and
// uses the VOTER_OUTGOING type instead (see VersionChangeReplicasDemotion for
// details on why we're not doing that any more).
//
// All voter types indicate a replica that participates in all raft activities,
// including voting for leadership and committing entries. Typically, this
// requires a majority of voters to reach a decision. In a joint config, two
// separate majorities are required: one from the set of replicas that have
// either type VOTER or VOTER_OUTOING or VOTER_DEMOTING, as well as that of the
// set of types VOTER and VOTER_INCOMING . For example, when type VOTER_FULL is
// assigned to replicas 1 and 2, while 3 is VOTER_OUTGOING and 4 is
// VOTER_INCOMING, then the two sets over which quorums need to be achieved are
// {1,2,3} and {1,2,4}. Thus, {1,2} is a quorum of both, {1,3} is a quorum of
// the first but not the second, {1,4} is a quorum of the second but not the
// first, and {3,4} is a quorum of neither.
enum ReplicaType {
  option (gogoproto.goproto_enum_prefix) = false;

  // VOTER_FULL indicates a replica that is a voter both in the
  // incoming and outgoing set.
  VOTER_FULL = 0;
  // VOTER_INCOMING indicates a voting replica that will be a
  // VOTER_FULL once the ongoing atomic replication change is finalized; that is,
  // it is in the process of being added. In practice, this replica type should
  // be treated like a VOTER_FULL.
  VOTER_INCOMING = 2;
  // VOTER_OUTGOING indicates a voting replica that will not be part
  // of the descriptor once the ongoing atomic replication change is finalized;
  // that is, it is in the process of being removed. In practice, a replica of
  // this type should be treated accordingly and no work should be assigned to
  // it.
  VOTER_OUTGOING = 3;
  // ...
}

Comments on API and interfaces

From pgwire/auth.go: comments on the top-level definition of a public API indicate the purpose of each function but also what interactions there may be between the functions/methods in the API.

// AuthConn is the interface used by the authenticator for interacting with the
// pgwire connection.
type AuthConn interface {
  // SendAuthRequest send a request for authentication information. After
  // calling this, the authenticator needs to call GetPwdData() quickly, as the
  // connection's goroutine will be blocked on providing us the requested data.
  SendAuthRequest(authType int32, data []byte) error
  // GetPwdData returns authentication info that was previously requested with
  // SendAuthRequest. The call blocks until such data is available.
  // An error is returned if the client connection dropped or if the client
  // didn't respect the protocol. After an error has been returned, GetPwdData()
  // cannot be called any more.
  GetPwdData() ([]byte, error)
  // AuthOK declares that authentication succeeded and provides a
  // unqualifiedIntSizer, to be returned by authenticator.authResult(). Future
  // authenticator.sendPwdData() calls fail.
  AuthOK(unqualifiedIntSizer)
  // AuthFail declares that authentication has failed and provides an error to
  // be returned by authenticator.authResult(). Future
  // authenticator.sendPwdData() calls fail. The error has already been written
  // to the client connection.
  AuthFail(err error)
  // Logf logs a message on the authentication log, if auth logs
  // are enabled.
  Logf(ctx context.Context, format string, args ...interface{})
}

Function comments

From pkg/server/admin.go: The top-level comment for a function outlines what the function does, any special cases it may have, and if possible examples.

If the function deviates from a Go idiom, a comment should explain/highlight that.

// Append appends the provided string and any number of query parameters.
// Instead of using normal placeholders (e.g. $1, $2), use meta-placeholder $.
// This method rewrites the query so that it uses proper placeholders.
//
// For example, suppose we have the following calls:
//
//   query.Append("SELECT * FROM foo WHERE a > $ AND a < $ ", arg1, arg2)
//   query.Append("LIMIT $", limit)
//
// The query is rewritten into:
//
//   SELECT * FROM foo WHERE a > $1 AND a < $2 LIMIT $3
//   /* $1 = arg1, $2 = arg2, $3 = limit */
//
// Note that this method does NOT return any errors. Instead, we queue up
// errors, which can later be accessed. Returning an error here would make
// query construction code exceedingly tedious.
func (q *sqlQuery) Append(s string, params ...interface{}) { ... }

Comments on phases inside a function body

From replica_command.go: comments highlight important phases/steps in a control flow and contain explanations about what motivates them. The depth (and explanatory power) of the comments should increase with the amount of work that was needed to design the code or fix its bugs.

func (r *Replica) executeAdminCommandWithDescriptor(
  ctx context.Context, updateDesc func(*roachpb.RangeDescriptor) error,
) *roachpb.Error {
  // Retry forever as long as we see errors we know will resolve.
  retryOpts := base.DefaultRetryOptions()
  // Randomize quite a lot just in case someone else also interferes with us
  // in a retry loop. Note that this is speculative; there wasn't an incident
  // that suggested this.
  retryOpts.RandomizationFactor = 0.5
  var lastErr error
  for retryable := retry.StartWithCtx(ctx, retryOpts); retryable.Next(); {
    // The replica may have been destroyed since the start of the retry loop.
    // We need to explicitly check this condition. Having a valid lease, as we
    // verify below, does not imply that the range still exists: even after a
    // range has been merged into its left-hand neighbor, its final lease
    // (i.e., the lease we have in r.mu.state.Lease) can remain valid
    // indefinitely.
    if _, err := r.IsDestroyed(); err != nil {
      return roachpb.NewError(err)
    }

    // Admin commands always require the range lease to begin (see
    // executeAdminBatch), but we may have lost it while in this retry loop.
    // Without the lease, a replica's local descriptor can be arbitrarily
    // stale, which will result in a ConditionFailedError. To avoid this, we
    // make sure that we still have the lease before each attempt.
    if _, pErr := r.redirectOnOrAcquireLease(ctx); pErr != nil {
      return pErr
    }

Comments on private structs

From cli/sql.go: even if a struct is not exported as a public API, it is generally useful to explain fields (and methods), to aid reasoning by a future reader/maintainer of the code.

// cliState defines the current state of the CLI during
// command-line processing.
//
// Note: options customizable via \set and \unset should be defined in
// sqlCtx or cliCtx instead, so that the configuration remains globals
// across multiple instances of cliState (e.g. across file inclusion
// with \i).
type cliState struct {
   ...
  // forwardLines is the array of lookahead lines. This gets
  // populated when there is more than one line of input
  // in the data read by ReadLine(), which can happen
  // when copy-pasting.
  forwardLines []string

  // partialLines is the array of lines accumulated so far in a
  // multi-line entry.
  partialLines []string

  // partialStmtsLen represents the number of entries in partialLines
  // parsed successfully so far. It grows larger than zero whenever 1)
  // syntax checking is enabled and 2) multi-statement (as opposed to
  // multi-line) entry starts, i.e. when the shell decides to continue
  // inputting statements even after a full statement followed by a
  // semicolon was read successfully. This is currently used for
  // multi-line transaction editing.
  partialStmtsLen int

  // concatLines is the concatenation of partialLines, computed during
  // doPrepareStatementLine and then reused in doRunStatements() and
  // doCheckStatement().
  concatLines string
...

CockroachDB