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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
