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