[llvm-dev] r338291 - Remove trailing space
Fāng-ruì Sòng via llvm-dev
llvm-dev at lists.llvm.org
Mon Jul 30 16:01:04 PDT 2018
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).
More information about the llvm-dev
mailing list