<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jul 30, 2018 at 6:17 PM, Fāng-ruì Sòng <span dir="ltr"><<a href="mailto:maskray@google.com" target="_blank">maskray@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On 2018-07-30, Aaron Ballman wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Mon, Jul 30, 2018 at 4:43 PM, Fāng-ruì Sòng <<a href="mailto:maskray@google.com" target="_blank">maskray@google.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Does this apply to only public headers (include/llvm include/llvm-c<br>
include/clang ...) or everything? (lib/**/*.{cpp,h})?<br>
</blockquote>
<br>
I've understood it applies to "everything" in that you should not<br>
commit large-scale NFC changes that don't have considerable benefit<br>
(for some definition of considerable benefit).<br>
</blockquote>
<br></span>
The benefits I can think of are:<br>
<br>
* Some editors are configured to highlight trailing whitespace. Before<br>
 the two cleanup commits, they will interfere reading.<br>
* Some editors are configured to remove whitespace (as Michael pointed<br>
 out). The removal will show up in diffs where revision authors have to undo<br>
 manually. For some out-of-tree users, if they have local patches but do not<br>
 strip trailing whitespace, related to settings of their code review system,<br>
 this could turn to whitespace errors.<span><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
e.g., if you're fixing<br>
a typo in an API and it hits a lot of files, that's fine because a<br>
typo in an API is pretty egregious, but don't clang-format a bunch of<br>
files and commit that because formatting isn't super critical and<br>
instead we migrate it more slowly as the code gets touched.<br>
</blockquote>
<br></span>
Thank you for raising these. I learned from your examples☺<span><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Mon, Jul 30, 2018 at 4:49 PM, Fāng-ruì Sòng <<a href="mailto:maskray@google.com" target="_blank">maskray@google.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Maybe not too terrible for out-of-tree projects. If they use git<br>
mirror, `git rebase` is smart enough to ignore changing lines with<br>
trailing whitespace (if not, there is git rebase<br>
-Xignore-space-at-eol). Some editors are configured with highlighting<br>
trailing spaces and these spaces will turn to eyesore...<br>
</blockquote>
<br>
Not everyone uses git; svn is still the official repository for the project.<br>
</blockquote>
<br></span>
I am not familiar with svn but stackoverflow tells me there is svn diff -x "--ignore-eol-style".<br>
This should also be available to other svn functionality.<br>
</blockquote></div><br></div><div class="gmail_extra">I don't disagree that there are pros and cons in the discussion and that we should consider them carefully, but I think there should be a discussion that happens with the community *before* landing this patch, even though it's a NFC patch. Please revert these patches and start a discussion thread on whether this is something the community would like to see committed or not.</div><div class="gmail_extra"><br></div><div class="gmail_extra">~Aaron</div></div>