On Mon, Jul 30, 2018 at 6:17 PM, Fāng-ruì Sòng <mask...@google.com> wrote:
> On 2018-07-30, Aaron Ballman wrote:
>
>> On Mon, Jul 30, 2018 at 4:43 PM, Fāng-ruì Sòng <mask...@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 <mask...@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.
~Aaron
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
