Update the coding standard about NFC changes and whitespace

[aaron.ballman]

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.

[nhaehnle]

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]

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]

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]

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]

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.

[aaron.ballman]

Updated based on review feedback.

[aaron.ballman]

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]

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.

[aaron.ballman]

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!

[probinson]

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

[aaron.ballman]

In D50055#1183277, @probinson wrote:

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]

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]

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]

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]

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]

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.

[aaron.ballman]

Updating based on review feedback.

[aaron.ballman]

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]

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]

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]

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]

In D50055#1188479, @chandlerc wrote:

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]

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]

docs/CodingStandards.rst

514–516

Thank you for sharing.

[aaron.ballman]

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

docs/CodingStandards.rst

 columns (for example) instead of 80 columns wouldn't add any significant value
 and would be detrimental to printing out code.  Also many other projects have
 standardized on 80 columns, so some people have already configured their editors
 for it (vs something else, like 90 columns).
 
 This is one of many contentious issues in coding standards, but it is not up for
 debate.
 
-Use Spaces Instead of Tabs
-^^^^^^^^^^^^^^^^^^^^^^^^^^
+Whitespace
+^^^^^^^^^^
 
 In all cases, prefer spaces to tabs in source files.  People have different
 preferred indentation levels, and different styles of indentation that they
 like; this is fine.  What isn't fine is that different editors/viewers expand
 tabs out to different tab stops.  This can cause your code to look completely
 unreadable, and it is not worth dealing with.
 
 As always, follow the `Golden Rule`_ above: follow the style of
 existing code if you are modifying and extending it.  If you like four spaces of
 indentation, **DO NOT** do that in the middle of a chunk of code with two spaces
 of indentation.  Also, do not reindent a whole source file: it makes for
 incredible diffs that are absolutely worthless.
 
+Do not commit changes that include trailing whitespace. If you find trailing
+whitespace in a file, do not remove it unless you're otherwise changing that
+line of code. Some common editors will automatically remove trailing whitespace
+when saving a file which causes unrelated changes to appear in diffs and
+commits.
+
 Indent Code Consistently
 ^^^^^^^^^^^^^^^^^^^^^^^^
 
 Okay, in your first year of programming you were told that indentation is
 important. If you didn't believe and internalize this then, now is the time.
 Just do it. With the introduction of C++11, there are some new formatting
 challenges that merit some suggestions to help have consistent, maintainable,
 and tool-friendly formatting and indentation.
 
 Format Lambdas Like Blocks Of Code
 """"""""""""""""""""""""""""""""""

docs/DeveloperPolicy.rst

    approval, submit a `patch`_ to `llvm-commits
    <http://lists.llvm.org/mailman/listinfo/llvm-commits>`_. When approved,
    you may commit it yourself.
 
 #. You are allowed to commit patches without approval which you think are
    obvious. This is clearly a subjective decision --- we simply expect you to
    use good judgement.  Examples include: fixing build breakage, reverting
    obviously broken patches, documentation/comment changes, any other minor
-   changes.
+   changes. Avoid committing formatting- or whitespace-only changes outside of
+   code you plan to make subsequent changes to. Also, try to separate
+   formatting or whitespace changes from functional changes, either by
+   correcting the format first (ideally) or afterward. Such changes should be
+   highly localized and the commit message should clearly state that the commit
+   is not intended to change functionality, usually by stating it is
+   :ref:`NFC <nfc>`.
 
 #. You are allowed to commit patches without approval to those portions of LLVM
    that you have contributed or maintain (i.e., have been assigned
    responsibility for), with the proviso that such commits must not break the
    build.  This is a "trust but verify" policy, and commits of this nature are
    reviewed after they are committed.
 
 #. Multiple violations of these policies or a single egregious violation may
    cause commit access to be revoked.
 
 In any case, your changes are still subject to `code review`_ (either before or
 after they are committed, depending on the nature of the change).  You are
 encouraged to review other peoples' patches as well, but you aren't required
 to do so.
 
 .. _discuss the change/gather consensus:
 
 Making a Major Change
 ---------------------
 
 When a developer begins a major new project with the aim of contributing it back
 to LLVM, they should inform the community with an email to the `llvm-dev
 <http://lists.llvm.org/mailman/listinfo/llvm-dev>`_ email list, to the extent
 possible. The reason for this is to:
 
 #. keep the community informed about future changes to LLVM,
 
 #. avoid duplication of effort by preventing multiple parties working on the
    same thing and not knowing about it, and
 
 #. ensure that any technical issues around the proposed work are discussed and

docs/Lexicon.rst

 M
 -
 
 **MC**
     Machine Code
 
 N
 -
+.. _nfc:
 
 **NFC**
   "No functional change". Used in a commit message to indicate that a patch
   is a pure refactoring/cleanup.
   Usually used in the first line, so it is visible without opening the
   actual commit email.
 
 O