<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jul 30, 2018 at 7:18 PM, Reid Kleckner via cfe-dev <span dir="ltr"><<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">- Please do not revert this change.</div></blockquote><div><br></div><div>I found out that reverting wouldn't help the situation I was worried about anyway, the damage was already done. Thankfully, the merge conflicts haven't been too awful.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>- Please avoid committing trailing whitespace in the first place. If that's not already a rule in LLVM's style, let's add it.<br>- In the future, please do not commit massive formatting changes to files that you are not otherwise editing.</div><div>- If you are editing a file and you find it convenient to reformat it, please do it separately before you send your code for review to avoid obscuring your changes.</div></div></blockquote><div><br></div><div>+1 to all of this. I will be proposing some edits to the developer policy to make this more clear for folks in the future.</div><div><br></div><div>~Aaron</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br><div class="gmail_quote"><div><div class="h5"><div dir="ltr">On Mon, Jul 30, 2018 at 4:01 PM Fāng-ruì Sòng via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br></div></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">I apologize that my two patches "Remove trailing space" (r338291 in clang, r338293 in llvm)<br>
are committed without a discussion happening within the community.<br>
<br>
Forward to llvm-dev and cfe-dev to see if they are what the community would like to see committed or not.<br>
<br>
The original discussion is at this thread<br>
<a href="http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20180730/236802.html" rel="noreferrer" target="_blank">http://lists.llvm.org/<wbr>pipermail/cfe-commits/Week-of-<wbr>Mon-20180730/236802.html</a><br>
<br>
On 2018-07-30, Aaron Ballman wrote:<br>
>On Mon, Jul 30, 2018 at 6:17 PM, Fāng-ruì Sòng <<a href="mailto:maskray@google.com" target="_blank">maskray@google.com</a>> wrote:<br>
><br>
> On 2018-07-30, Aaron Ballman wrote:<br>
><br>
> 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>><br>
> wrote:<br>
><br>
> Does this apply to only public headers (include/llvm include/llvm-c<br>
> include/clang ...) or everything? (lib/**/*.{cpp,h})?<br>
><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>
><br>
> 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<br>
> undo<br>
> manually. For some out-of-tree users, if they have local patches but do<br>
> not<br>
> strip trailing whitespace, related to settings of their code review<br>
> system,<br>
> this could turn to whitespace errors.<br>
><br>
> 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>
><br>
> Thank you for raising these. I learned from your examples☺<br>
><br>
> 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>><br>
> wrote:<br>
><br>
> 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<br>
> highlighting<br>
> trailing spaces and these spaces will turn to eyesore...<br>
><br>
> Not everyone uses git; svn is still the official repository for the<br>
> project.<br>
><br>
> I am not familiar with svn but stackoverflow tells me there is svn diff -x<br>
> "--ignore-eol-style".<br>
> This should also be available to other svn functionality.<br>
><br>
>I don't disagree that there are pros and cons in the discussion and that we<br>
>should consider them carefully, but I think there should be a discussion that<br>
>happens with the community *before* landing this patch, even though it's a NFC<br>
>patch. Please revert these patches and start a discussion thread on whether<br>
>this is something the community would like to see committed or not.<br>
<br>
I want to collect some responses before doing the revert, in case the<br>
revert actually makes things worse (people have to rebuild since these<br>
files have changed again).<br></div></div>
______________________________<wbr>_________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-dev</a><br>
</blockquote></div>
<br>______________________________<wbr>_________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-dev</a><br>
<br></blockquote></div><br></div></div>