[cfe-dev] r338291 - Remove trailing space
Aaron Ballman via cfe-dev
cfe-dev at lists.llvm.org
Tue Jul 31 04:41:39 PDT 2018
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/b9febfdf/attachment.html>
More information about the cfe-dev
mailing list