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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 2 inline comments as done.
aaron.ballman 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
----------------
hfinkel wrote:
> 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).
> 
Good point; I've made those changes.


================
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
----------------
chandlerc wrote:
> hubert.reinterpretcast wrote:
> > hfinkel wrote:
> > > 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?
> > > 
> > 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.
> 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.
Good suggestions! This made the wording short enough that I rolled it in with the "obvious" wording above.


https://reviews.llvm.org/D50055