[cfe-dev] r338291 - Remove trailing space

[Aaron Ballman via cfe-dev]

Thank you, everyone, for the discussion on this -- I've put up a review at
https://reviews.llvm.org/D50055 for the coding standard changes, if you're
interested.

~Aaron

On Tue, Jul 31, 2018 at 7:41 AM, Aaron Ballman <aaron at aaronballman.com>
wrote:

> 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/cfe-dev/attachments/20180731/e03d5aec/attachment.html>