[cfe-dev] r338291 - Remove trailing space

[Reid Kleckner via cfe-dev]

- 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/cfe-dev/attachments/20180730/f1941749/attachment.html>