Page MenuHomePhabricator

Update the coding standard about NFC changes and whitespace
ClosedPublic

Authored by aaron.ballman on Jul 31 2018, 5:36 AM.

Details

Reviewers
chandlerc
rnk
Summary

We recently had a large-scale commit of NFC whitespace changes land (r338291) and it was recognized that we don't have any explicit statements in our coding standard about not introducing trailing whitespace when committing code, and not committing large-scale NFC changes without prior review due to needless churn issues.

This patch adds the new information to the coding standard to document community intent as best I understand it.

Diff Detail

Event Timeline

aaron.ballman created this revision.Jul 31 2018, 5:36 AM
nhaehnle added inline comments.
docs/CodingStandards.rst
519–520

Is that really true, though? Whitespace changes, sure, but there are plenty of non-whitespace changes that are arguably NFC (and often labeled as such) but merit some discussion.

aaron.ballman added inline comments.Jul 31 2018, 6:02 AM
docs/CodingStandards.rst
519–520

I believe it's true, yes, but perhaps I've not explained it well enough. We've always had the position that it's fine to fix typos, rename a local variable to be more clear, fix 80-col limit violations, trailing whitespace, etc as a one-off commit that doesn't require review. It's not that *no* NFC changes require discussion, it's that most NFC changes should be small and obvious enough to not warrant discussion.

If you have a suggestion for how to make this more clear, I'd appreciate it.

probinson added inline comments.
docs/CodingStandards.rst
519–520

For non-formatting NFC changes, there's the "obviousness" rule. For formatting, we prefer you do it (a) by clang-format-diff for just your patch, or (b) clang-format a file you are about to do more extensive work on, as a separate commit prior to the real patch. For things like line endings and trailing whitespace, people do that all the time, but it's usually just a file or so at a time and nobody minds.

I think leading this section with "it's okay" and then adding exceptions later might tend to give the wrong impression. It's more like, there are different cases, with different considerations.

nhaehnle added inline comments.Jul 31 2018, 8:36 AM
docs/CodingStandards.rst
519–520

I think leading this section with "it's okay" and then adding exceptions later might tend to give the wrong impression. It's more like, there are different cases, with different considerations.

Yes, that's pretty much where I was coming from.

rnk added a subscriber: MaskRay.Jul 31 2018, 11:08 AM
rnk added inline comments.
docs/CodingStandards.rst
519–520

This is really an addendum to the "obviousness" rule in DeveloperPolicy.rst doc, which is probably where @MaskRay got the idea that "minor" changes are OK without review. I'd put this clarification there. We could add a sentence there requesting that people not commit large formatting changes unless it is local to the code that they happen to be reading or writing.

Updated based on review feedback.

aaron.ballman added inline comments.Jul 31 2018, 11:18 AM
docs/CodingStandards.rst
519–520

I tried to address this by moving it to the Developer Policy and rewording it slightly to clarify there are different NFC situations.

rnk accepted this revision.Jul 31 2018, 11:31 AM

I'm happy with this, but since this is a policy change, let's wait a day to see if anyone else wants to chime in.

This revision is now accepted and ready to land.Jul 31 2018, 11:31 AM
In D50055#1183032, @rnk wrote:

I'm happy with this, but since this is a policy change, let's wait a day to see if anyone else wants to chime in.

Definitely not a huge rush to get this in, thank you for the review!

I think you forgot to subscribe llvm-commits to this review?

I think you forgot to subscribe llvm-commits to this review?

Oh shoot, good catch! I've added llvm-commits and cfe-commits both. Thank you for pointing that out.

chandlerc added inline comments.Jul 31 2018, 2:37 PM
docs/DeveloperPolicy.rst
395–408

I think this is a much broader statement than is warranted to address the specific problem. And I'm not completely comfortable with it.

I don't think guidance around "no functional change" is the right way to give guidance about what is or isn't "obvious" and fine to commit with post-commit review personally.

I'd really suggest just being direct about *formatting* and whitespace changes, and specifically suggest that they not be made unless the file or code region in question is an area that the author is planning subsequent changes to.

aaron.ballman added inline comments.Aug 1 2018, 1:41 PM
docs/DeveloperPolicy.rst
395–408

We talk about formatting and whitespace in the CodingStandards document, but we talk about obviousness and post-commit review in DeveloperPolicy. Where would you like these new words to live? To me, they're more about the policy and less about the coding standard -- the coding standard says what the code should look like and the policy says what you should and should not do procedurally, but then I feel the need to tie it back to "obviousness". How about this in the developer policy:

The Obviousness of Formatting Changes
-------------------------------------

While formatting and whitespace changes may be "obvious", they can also create
needless churn that causes difficulties for out-of-tree users carrying local
patches. Do not commit formatting or whitespace changes unless they are related
to a file or code region that you intend to make subsequent changes to. The
formatting and whitespace changes should be highly localized, committed before
you begin functionality-changing work on the code region, and the commit message
should clearly state that the commit is not intended to change functionality,
usually by stating it is :ref:`NFC <nfc>`.


If you wish to make a change to formatting or whitespace that touches an entire
library or code base, please seek pre-commit approval first.
hfinkel added a subscriber: hfinkel.Aug 1 2018, 2:08 PM
hfinkel added inline comments.
docs/CodingStandards.rst
512

This statement is confusing (mostly because it has two reasonable interpretations and I think you actually mean both). We should say two separate things:

  1. As a coding guideline, make sure that lines don't have trailing whitespace.
  2. If such whitespace exists, don't remove it unless you're otherwise changing that line of code (and here we can caution people about their editors).
docs/DeveloperPolicy.rst
395–408

I agree with @chandlerc about the current proposed text, and I think that this is better. I wonder if we want to insist on separating the commits, of if, combined localized commits are okay?

hubert.reinterpretcast added inline comments.
docs/DeveloperPolicy.rst
395–408

It depends on how much noise there is when combining the commits; and when evaluating for that, we have to remember that people use different diff tools.

chandlerc added inline comments.Aug 1 2018, 2:49 PM
docs/DeveloperPolicy.rst
395–408

I like Hal's separation in the other comment.

Here, I tihnk we can address all of this by making this more of a (strong) suggestion and not a hard rule.

"Avoid committing formatting or whitespace only changes outside of code you plan to make subsequent changes to." or something similar.

Then it also becomes natural to suggest:

"Also, try to separate formatting or whitespace changes from functional changes, either by correcting the format first (ideally) or afterward."

I think you can also shorten some of the discussion along these lines.

Updating based on review feedback.

aaron.ballman marked 2 inline comments as done.Aug 2 2018, 4:53 AM
aaron.ballman added inline comments.
docs/CodingStandards.rst
512

Good point; I've made those changes.

docs/DeveloperPolicy.rst
395–408

Good suggestions! This made the wording short enough that I rolled it in with the "obvious" wording above.

chandlerc accepted this revision.Aug 4 2018, 12:24 AM

This looks really good to me and seems like a nice clarification of historical practice. =D Thanks so much for driving an actual documentation update for folks that simply would never know about these practices otherwise, I think it will help folks a lot.

Maybe double check with Reid and/or Hal to make sure we've all ended up on the same page before landing though.

Meinersbur added inline comments.
docs/CodingStandards.rst
514–516

Just to note that this policy will prohibit the use of remove trailing-whitespace feature in editors since it makes it impossible to edit the file without also 'editing' any unrelated whitespace. At the same time, since not being able to use the feature, I risk committing trailing whitespace myself (that is, some additional steps are necessary: I use 'git clang-format' which should remove traling whitespace only on edited lines and 'git diff' shows trailing whitespace in red; these are still additional steps that are easy to miss)

chandlerc added inline comments.Aug 4 2018, 8:34 PM
docs/CodingStandards.rst
514–516

I also have editor settings that risk violating this, but I just reduce my patdh before submitting. This is a touch annoying with svn, but with got it is pretty simple to use git add -p and ignore the unnecessary removals if trailing whitespace

rnk added a comment.Aug 6 2018, 11:22 AM

Maybe double check with Reid and/or Hal to make sure we've all ended up on the same page before landing though.

lgtm, thanks

JDevlieghere added inline comments.Aug 7 2018, 5:28 AM
docs/CodingStandards.rst
514–516

I had the same workflow as Chandler but that became rather tedious for the LLDB repo where there's a lot of trailing whitespace. I ended up adding an alias to my git config that only stages non-whitespace changes: anw = !sh -c 'git diff -U0 -w --no-color "$@" | git apply --cached --ignore-whitespace --unidiff-zero -'. It's far from optimal but it works pretty well.

Meinersbur added inline comments.Aug 7 2018, 8:31 AM
docs/CodingStandards.rst
514–516

Thank you for sharing.

aaron.ballman marked 2 inline comments as done.

Thank you for the review and discussion -- I've commit in r339455.