llvm dev - Jul 2018 - [cfe-dev] r338291 - Remove trailing space

I apologize that my two patches "Remove trailing space" (r338291 in
clang, r338293 in llvm)
are committed without a discussion happening within the community.

Forward to llvm-dev and cfe-dev to see if they are what the community would like
to see committed or not.

The original discussion is at this thread
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20180730/236802.html

On 2018-07-30, Aaron Ballman wrote:
>On Mon, Jul 30, 2018 at 6:17 PM, Fāng-ruì Sòng <maskray at google.com>
wrote:
>
>    On 2018-07-30, Aaron Ballman wrote:
>
>        On Mon, Jul 30, 2018 at 4:43 PM, Fāng-ruì Sòng <maskray at
google.com>
>        wrote:
>
>            Does this apply to only public headers (include/llvm
include/llvm-c
>            include/clang ...) or everything? (lib/**/*.{cpp,h})?
>
>        I've understood it applies to "everything" in that you
should not
>        commit large-scale NFC changes that don't have considerable
benefit
>        (for some definition of considerable benefit).
>
>    The benefits I can think of are:
>
>    * Some editors are configured to highlight trailing whitespace. Before
>     the two cleanup commits, they will interfere reading.
>    * Some editors are configured to remove whitespace (as Michael pointed
>     out). The removal will show up in diffs where revision authors have to
>    undo
>     manually. For some out-of-tree users, if they have local patches but do
>    not
>     strip trailing whitespace, related to settings of their code review
>    system,
>     this could turn to whitespace errors.
>
>        e.g., if you're fixing
>        a typo in an API and it hits a lot of files, that's fine because
a
>        typo in an API is pretty egregious, but don't clang-format a
bunch of
>        files and commit that because formatting isn't super critical
and
>        instead we migrate it more slowly as the code gets touched.
>
>    Thank you for raising these. I learned from your examples☺
>
>        On Mon, Jul 30, 2018 at 4:49 PM, Fāng-ruì Sòng <maskray at
google.com>
>        wrote:
>
>            Maybe not too terrible for out-of-tree projects. If they use git
>            mirror, `git rebase` is smart enough to ignore changing lines
with
>            trailing whitespace (if not, there is git rebase
>            -Xignore-space-at-eol). Some editors are configured with
>            highlighting
>            trailing spaces and these spaces will turn to eyesore...
>
>        Not everyone uses git; svn is still the official repository for the
>        project.
>
>    I am not familiar with svn but stackoverflow tells me there is svn diff
-x
>    "--ignore-eol-style".
>    This should also be available to other svn functionality.
>
>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.
I want to collect some responses before doing the revert, in case the
revert actually makes things worse (people have to rebuild since these
files have changed again).

- Please do not revert this change.
- Please avoid committing trailing whitespace in the first place. If that's
not already a rule in LLVM's style, let's add it.
- In the future, please do not commit massive formatting changes to files
that you are not otherwise editing.
- 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.

On Mon, Jul 30, 2018 at 4:01 PM Fāng-ruì Sòng via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> I apologize that my two patches "Remove trailing space" (r338291
in clang,
> r338293 in llvm)
> are committed without a discussion happening within the community.
>
> Forward to llvm-dev and cfe-dev to see if they are what the community
> would like to see committed or not.
>
> The original discussion is at this thread
>
>
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20180730/236802.html
>
> On 2018-07-30, Aaron Ballman wrote:
> >On Mon, Jul 30, 2018 at 6:17 PM, Fāng-ruì Sòng <maskray at
google.com>
> wrote:
> >
> >    On 2018-07-30, Aaron Ballman wrote:
> >
> >        On Mon, Jul 30, 2018 at 4:43 PM, Fāng-ruì Sòng <
> maskray at google.com>
> >        wrote:
> >
> >            Does this apply to only public headers (include/llvm
> include/llvm-c
> >            include/clang ...) or everything? (lib/**/*.{cpp,h})?
> >
> >        I've understood it applies to "everything" in
that you should not
> >        commit large-scale NFC changes that don't have considerable
> benefit
> >        (for some definition of considerable benefit).
> >
> >    The benefits I can think of are:
> >
> >    * Some editors are configured to highlight trailing whitespace.
Before
> >     the two cleanup commits, they will interfere reading.
> >    * Some editors are configured to remove whitespace (as Michael
pointed
> >     out). The removal will show up in diffs where revision authors
have
> to
> >    undo
> >     manually. For some out-of-tree users, if they have local patches
but
> do
> >    not
> >     strip trailing whitespace, related to settings of their code
review
> >    system,
> >     this could turn to whitespace errors.
> >
> >        e.g., if you're fixing
> >        a typo in an API and it hits a lot of files, that's fine
because a
> >        typo in an API is pretty egregious, but don't clang-format
a
> bunch of
> >        files and commit that because formatting isn't super
critical and
> >        instead we migrate it more slowly as the code gets touched.
> >
> >    Thank you for raising these. I learned from your examples☺
> >
> >        On Mon, Jul 30, 2018 at 4:49 PM, Fāng-ruì Sòng <
> maskray at google.com>
> >        wrote:
> >
> >            Maybe not too terrible for out-of-tree projects. If they
use
> git
> >            mirror, `git rebase` is smart enough to ignore changing
lines
> with
> >            trailing whitespace (if not, there is git rebase
> >            -Xignore-space-at-eol). Some editors are configured with
> >            highlighting
> >            trailing spaces and these spaces will turn to eyesore...
> >
> >        Not everyone uses git; svn is still the official repository for
> the
> >        project.
> >
> >    I am not familiar with svn but stackoverflow tells me there is svn
> diff -x
> >    "--ignore-eol-style".
> >    This should also be available to other svn functionality.
> >
> >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.
>
> I want to collect some responses before doing the revert, in case the
> revert actually makes things worse (people have to rebuild since these
> files have changed again).
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180730/f1941749/attachment.html>

On Tue, 31 Jul 2018 at 00:19, Reid Kleckner via cfe-dev
<cfe-dev at lists.llvm.org> wrote:
> - Please do not revert this change.
I'd prefer not reverting too. We've already had to deal with the
conflict and reverting would just repeat it, without fixing blame.

Tim.

On Mon, Jul 30, 2018 at 7:18 PM, Reid Kleckner via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> - Please do not revert this change.
>
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.

> - Please avoid committing trailing whitespace in the first place. If
> that's not already a rule in LLVM's style, let's add it.
> - In the future, please do not commit massive formatting changes to files
> that you are not otherwise editing.
> - 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.
>
+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.

~Aaron

>
> On Mon, Jul 30, 2018 at 4:01 PM Fāng-ruì Sòng via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> I apologize that my two patches "Remove trailing space"
(r338291 in
>> clang, r338293 in llvm)
>> are committed without a discussion happening within the community.
>>
>> Forward to llvm-dev and cfe-dev to see if they are what the community
>> would like to see committed or not.
>>
>> The original discussion is at this thread
>> http://lists.llvm.org/pipermail/cfe-commits/Week-of-
>> Mon-20180730/236802.html
>>
>> On 2018-07-30, Aaron Ballman wrote:
>> >On Mon, Jul 30, 2018 at 6:17 PM, Fāng-ruì Sòng <maskray at
google.com>
>> wrote:
>> >
>> >    On 2018-07-30, Aaron Ballman wrote:
>> >
>> >        On Mon, Jul 30, 2018 at 4:43 PM, Fāng-ruì Sòng <
>> maskray at google.com>
>> >        wrote:
>> >
>> >            Does this apply to only public headers (include/llvm
>> include/llvm-c
>> >            include/clang ...) or everything? (lib/**/*.{cpp,h})?
>> >
>> >        I've understood it applies to "everything" in
that you should not
>> >        commit large-scale NFC changes that don't have
considerable
>> benefit
>> >        (for some definition of considerable benefit).
>> >
>> >    The benefits I can think of are:
>> >
>> >    * Some editors are configured to highlight trailing whitespace.
>> Before
>> >     the two cleanup commits, they will interfere reading.
>> >    * Some editors are configured to remove whitespace (as Michael
>> pointed
>> >     out). The removal will show up in diffs where revision authors
have
>> to
>> >    undo
>> >     manually. For some out-of-tree users, if they have local
patches
>> but do
>> >    not
>> >     strip trailing whitespace, related to settings of their code
review
>> >    system,
>> >     this could turn to whitespace errors.
>> >
>> >        e.g., if you're fixing
>> >        a typo in an API and it hits a lot of files, that's
fine because
>> a
>> >        typo in an API is pretty egregious, but don't
clang-format a
>> bunch of
>> >        files and commit that because formatting isn't super
critical and
>> >        instead we migrate it more slowly as the code gets touched.
>> >
>> >    Thank you for raising these. I learned from your examples☺
>> >
>> >        On Mon, Jul 30, 2018 at 4:49 PM, Fāng-ruì Sòng <
>> maskray at google.com>
>> >        wrote:
>> >
>> >            Maybe not too terrible for out-of-tree projects. If
they use
>> git
>> >            mirror, `git rebase` is smart enough to ignore changing
>> lines with
>> >            trailing whitespace (if not, there is git rebase
>> >            -Xignore-space-at-eol). Some editors are configured
with
>> >            highlighting
>> >            trailing spaces and these spaces will turn to
eyesore...
>> >
>> >        Not everyone uses git; svn is still the official repository
for
>> the
>> >        project.
>> >
>> >    I am not familiar with svn but stackoverflow tells me there is
svn
>> diff -x
>> >    "--ignore-eol-style".
>> >    This should also be available to other svn functionality.
>> >
>> >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.
>>
>> I want to collect some responses before doing the revert, in case the
>> revert actually makes things worse (people have to rebuild since these
>> files have changed again).
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180731/b9febfdf/attachment.html>