Suggestion: Bug links in comments may mean comment could be better

[Peter Kasting]

If you never write comments with bug links in them, you can ignore this.

I commonly see comments like:

  // Avoid crash.  crbug.com/12345

This can be problematic for a few reasons.

• Bug links require the reader to go look elsewhere, so they're not reading the explanation in the context of looking at the code.  This makes it harder to understand the code at a glance. Depending on the complexity involved, this can make full comprehension require repeatedly switching between code and bug.
  
• Frequently bugs are long, have many comments, get spurious comments added later, morph, etc.  Even for simple bugs, the original author knows better than the later reader what pertinent information should be distilled as the rationale for the code.  This just gets more true as the bug gets longer and more complicated.
  
• When later modifying code, it's a lot more obvious when a change makes a local comment obsolete (and thus it needs updating) than when the change means a linked bug is no longer applicable, or (worse) only partly applicable.

I have not yet seen such a comment that wasn't better when the author replaced the bug link with a concise but complete description of what was happening and why the following code was the correct fix.

So next time you're tempted to include a bug link, ask yourself if you could do even better by writing a really good comment to keep all the pertinent information local.

PK

[Darin Fisher]

A good informative comment sounds great, but maybe the recommendation should be to do that in addition to providing the bug link. The bug link can provide other value. It may be blocking another bug, provide links to crash data, etc. Bug links are great for code archaeology :)

-Darin

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Peter Kasting]

On Tue, Jul 19, 2016 at 5:14 PM, Darin Fisher <da...@chromium.org> wrote:

A good informative comment sounds great, but maybe the recommendation should be to do that in addition to providing the bug link. The bug link can provide other value. It may be blocking another bug, provide links to crash data, etc. Bug links are great for code archaeology :)

There's no rule against bug links, so people are welcome to do what they think is clearest.  That said, I've never run into a comment that has both (a) such perfectly sufficient description locally that looking elsewhere is unnecessary to completely understand all relevant context, and (b) a link to a bug that has interesting additional detail and doesn't require nontrivial effort to parse out precisely what that information is and how it should be understood and applied.

I'm sure it's possible, but in my experience bug links end up undercutting (a) and usually failing to provide (b).

PK

[John Abd-El-Malek]

+1 to always leaving bug links (in addition to good comment). For example, I've found them helpful for removing dead or debugging code by following the bug. Other times it's useful when a bug has a workaround across multiple sites to be able to search cs for the bug id.

--

[Michael Giuffrida]

On Tue, Jul 19, 2016 at 5:22 PM 'Peter Kasting' via Chromium-dev <chromi...@chromium.org> wrote:

On Tue, Jul 19, 2016 at 5:14 PM, Darin Fisher <da...@chromium.org> wrote:

A good informative comment sounds great, but maybe the recommendation should be to do that in addition to providing the bug link. The bug link can provide other value. It may be blocking another bug, provide links to crash data, etc. Bug links are great for code archaeology :)

There's no rule against bug links, so people are welcome to do what they think is clearest.  That said, I've never run into a comment that has both (a) such perfectly sufficient description locally that looking elsewhere is unnecessary to completely understand all relevant context, and (b) a link to a bug that has interesting additional detail and doesn't require nontrivial effort to parse out precisely what that information is and how it should be understood and applied. 

Not sure (a) and (b) should be the litmus test for every comment. The comments with bug links I see and use most often fail both criteria:

// TODO(<username>): [remove workaround|update check|combine with foo|<something else>] when crbug.com/<12345> fix lands.

Without any comments, someone is likely to see the weird hack/workaround/whatever-else-that-needs-explaining and "fix" it. Without a bug link, the comments become awkward:

// TODO(<username>): remove workaround when workaround is no longer necessary, [insert a bunch of redundant explanation here and elsewhere]

and whoever fixes the bug can't just grep the code to figure out what can be simplified.

In fact, I haven't encountered many bug links that aren't of the above type; can you provide examples of what's unhelpful (anonymized if you like)?

 

I'm sure it's possible, but in my experience bug links end up undercutting (a) and usually failing to provide (b).

PK

--

[Peter Kasting]

On Tue, Jul 19, 2016 at 7:27 PM, Michael Giuffrida <mich...@chromium.org> wrote:

Without any comments, someone is likely to see the weird hack/workaround/whatever-else-that-needs-explaining and "fix" it. Without a bug link, the comments become awkward:

// TODO(<username>): remove workaround when workaround is no longer necessary, [insert a bunch of redundant explanation here and elsewhere]

and whoever fixes the bug can't just grep the code to figure out what can be simplified.

In fact, I haven't encountered many bug links that aren't of the above type; can you provide examples of what's unhelpful (anonymized if you like)?

Generally, bug links on TODO comments aren't as problematic, especially the sort of "wait for bug X to be fixed" comments you mention.  I've left such comments myself.

It's bug links on non-TODO comments, that intend to be permanent explanations for something, where I often find the comment could be improved.

I have some examples, but after looking at them I don't think it's very obvious to someone not already reading the code in context what the code means, what the comments should say, whether one comment is better than another, etc., so I'm intentionally not posting them.

The point of this thread was not to say that all bug links are inherently bad.  It was mostly to point out that we could write better comments in general, and one rule of thumb is that if you're leaving a bug link, simply think briefly as to whether you could make your comment even better.  If in the end you wind up with something that still has a bug link, so be it.  It's up to the author and reviewer to decide what's best.

PK