[PATCH] D50055: Update the coding standard about NFC changes and whitespace

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.


================
Comment at: docs/CodingStandards.rst:512
 
+Do not commit changes that include trailing whitespace. Some common editors will
+automatically remove trailing whitespace when saving a file which causes
----------------
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).



================
Comment at: docs/DeveloperPolicy.rst:395-408
+Commits with No Functional Change
+---------------------------------
+
+It may be permissible to commit changes without prior review when the changes
+have no semantic impact on the code if the changes being made are obvious and
+not invasive. For instance, removing trailing whitespace from a line, fixing a
+line ending to be consistent with the rest of the file, fixing a typo, code
----------------
aaron.ballman wrote:
> chandlerc wrote:
> > 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.
> 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.
> ```
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?



https://reviews.llvm.org/D50055