[RFC] Developing a policy for diagnostic updates

[cjdb]

It would be good for new diagnostics and updates to existing ones to show an example of the changes in the commit message. This will help with both:

• reviews: -verify and FileCheck expectations aren’t great for visualising diagnostics, and
• revision archaeology: it’ll demonstrate what we previously had and why the change is an improvement.

The examples should probably be what we’re adding as tests, but with actual Clang diagnostics underneath. D153359 has a good example of what I’ve got in mind.

[AaronBallman]

I think we should encourage this, but I’m not certain where it lands in terms of policy. Are you thinking of adding some words to the developer policy, or something more informal?

I think for more substantial diagnostic changes (not typo fixes or rewordings), having an example in the release notes is a great idea. That’s a good way to communicate the improvements to users.

In terms of commit messages and reviews, I’m less concerned there’s a problem that needs solving. The test coverage needs to be sufficient to demonstrate the benefit of the changes to reviewers, and while -verify and FileCheck aren’t as easy as having the information spoon-fed in an explicit way, they probably suffice enough that we don’t want to be too prescriptive here. (I wouldn’t want to hold up a review over not having that information in the summary, for instance.)

[cjdb]

@AaronBallman

I think we should encourage this, but I’m not certain where it lands in terms of policy. Are you thinking of adding some words to the developer policy, or something more informal?

Informal seems fine to me. Getting a bit of consensus

@AaronBallman

I think for more substantial diagnostic changes (not typo fixes or rewordings), having an example in the release notes is a great idea. That’s a good way to communicate the improvements to users.

Release notes make more sense with that in mind.

@AaronBallman

In terms of commit messages and reviews, I’m less concerned there’s a problem that needs solving. The test coverage needs to be sufficient to demonstrate the benefit of the changes to reviewers, and while -verify and FileCheck aren’t as easy as having the information spoon-fed in an explicit way, they probably suffice enough that we don’t want to be too prescriptive here. (I wouldn’t want to hold up a review over not having that information in the summary, for instance.)

So the reason I made this post is because I’d been struggling to understand what a handful of recent diagnostic reviews were trying to achieve, and D153359 happened to communicate the information in a way that very clearly helped me understand it. Most of these changes involved diagnostics that span a few lines, so it was probably difficult for me to hold all the pieces in my head at the same time.

I’m less interested in it being in the commit message, and more interested in it being associated with the commit (i.e. not just a comment in Phab). Release notes meet this criterion as well.

[AaronBallman]

@cjdb

Informal seems fine to me. Getting a bit of consensus

Fantastic, SGTM!

@cjdb

So the reason I made this post is because I’d been struggling to understand what a handful of recent diagnostic reviews were trying to achieve, and D153359 happened to communicate the information in a way that very clearly helped me understand it. Most of these changes involved diagnostics that span a few lines, so it was probably difficult for me to hold all the pieces in my head at the same time.

I’m less interested in it being in the commit message, and more interested in it being associated with the commit (i.e. not just a comment in Phab). Release notes meet this criterion as well.

Excellent, so it sounds like we basically should be reminding code reviewers that release notes should be expressive and show diagnostic changes when possible?

Btw, to be clear, it’s always reasonable when reviewing code to ask for examples of what the ramifications of the patch are. Feel free to ask for more details any time you feel they would help you provide a higher quality review.