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

2018-08-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman closed this revision.
aaron.ballman marked 2 inline comments as done.
aaron.ballman added a comment.

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


https://reviews.llvm.org/D50055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-08-07 Thread Michael Kruse via Phabricator via cfe-commits
Meinersbur added inline comments.



Comment at: docs/CodingStandards.rst:514-516
+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.

JDevlieghere wrote:
> chandlerc wrote:
> > Meinersbur wrote:
> > > 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)
> > 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 
> 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. 
Thank you for sharing.


https://reviews.llvm.org/D50055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-08-07 Thread Jonas Devlieghere via Phabricator via cfe-commits
JDevlieghere added inline comments.



Comment at: docs/CodingStandards.rst:514-516
+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.

chandlerc wrote:
> Meinersbur wrote:
> > 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)
> 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 
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. 


https://reviews.llvm.org/D50055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-08-06 Thread Reid Kleckner via Phabricator via cfe-commits
rnk added a comment.

In https://reviews.llvm.org/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


https://reviews.llvm.org/D50055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-08-04 Thread Chandler Carruth via Phabricator via cfe-commits
chandlerc added inline comments.



Comment at: docs/CodingStandards.rst:514-516
+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.

Meinersbur wrote:
> 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)
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 


https://reviews.llvm.org/D50055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-08-04 Thread Michael Kruse via Phabricator via cfe-commits
Meinersbur added inline comments.



Comment at: docs/CodingStandards.rst:514-516
+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.

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)


https://reviews.llvm.org/D50055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-08-04 Thread Chandler Carruth via Phabricator via cfe-commits
chandlerc accepted this revision.
chandlerc added a comment.

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.


https://reviews.llvm.org/D50055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-08-02 Thread 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 `.
> > > > 
> > > > 
> > > > 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-08-02 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman updated this revision to Diff 158731.
aaron.ballman added a comment.

Updating based on review feedback.


https://reviews.llvm.org/D50055

Files:
  docs/CodingStandards.rst
  docs/DeveloperPolicy.rst
  docs/Lexicon.rst


Index: docs/Lexicon.rst
===
--- docs/Lexicon.rst
+++ docs/Lexicon.rst
@@ -185,6 +185,7 @@
 
 N
 -
+.. _nfc:
 
 **NFC**
   "No functional change". Used in a commit message to indicate that a patch
Index: docs/DeveloperPolicy.rst
===
--- docs/DeveloperPolicy.rst
+++ docs/DeveloperPolicy.rst
@@ -376,7 +376,13 @@
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 `.
 
 #. You are allowed to commit patches without approval to those portions of LLVM
that you have contributed or maintain (i.e., have been assigned
Index: docs/CodingStandards.rst
===
--- docs/CodingStandards.rst
+++ docs/CodingStandards.rst
@@ -494,8 +494,8 @@
 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
@@ -509,6 +509,12 @@
 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
 
 


Index: docs/Lexicon.rst
===
--- docs/Lexicon.rst
+++ docs/Lexicon.rst
@@ -185,6 +185,7 @@
 
 N
 -
+.. _nfc:
 
 **NFC**
   "No functional change". Used in a commit message to indicate that a patch
Index: docs/DeveloperPolicy.rst
===
--- docs/DeveloperPolicy.rst
+++ docs/DeveloperPolicy.rst
@@ -376,7 +376,13 @@
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 `.
 
 #. You are allowed to commit patches without approval to those portions of LLVM
that you have contributed or maintain (i.e., have been assigned
Index: docs/CodingStandards.rst
===
--- docs/CodingStandards.rst
+++ docs/CodingStandards.rst
@@ -494,8 +494,8 @@
 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
@@ -509,6 +509,12 @@
 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
 
 
___
cfe-commits mailing list

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

2018-08-01 Thread Chandler Carruth via Phabricator via cfe-commits
chandlerc added inline comments.



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

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 `.
> > > 
> > > 
> > > 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.


https://reviews.llvm.org/D50055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-08-01 Thread Hubert Tong via Phabricator via cfe-commits
hubert.reinterpretcast added inline comments.



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

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 `.
> > 
> > 
> > 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.


https://reviews.llvm.org/D50055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-08-01 Thread 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 `.
> 
> 
> 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-08-01 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments.



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:
> 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 `.


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.
```


https://reviews.llvm.org/D50055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-07-31 Thread Chandler Carruth via Phabricator via cfe-commits
chandlerc added inline comments.



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

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.


https://reviews.llvm.org/D50055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-07-31 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added subscribers: llvm-commits, cfe-commits.
aaron.ballman added a comment.

In https://reviews.llvm.org/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.


https://reviews.llvm.org/D50055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits