Re: r338291 - Remove trailing space

2018-07-30 Thread Aaron Ballman via cfe-commits
On Mon, Jul 30, 2018 at 6:17 PM, Fāng-ruì Sòng wrote: > On 2018-07-30, Aaron Ballman wrote: > >> On Mon, Jul 30, 2018 at 4:43 PM, Fāng-ruì Sòng >> wrote: >> >>> Does this apply to only public headers (include/llvm include/llvm-c >>> include/clang ...) or everything? (lib/**/*.{cpp,h})? >>> >> >>

Re: r338291 - Remove trailing space

2018-07-30 Thread Fāng-ruì Sòng via cfe-commits
On 2018-07-30, Aaron Ballman wrote: On Mon, Jul 30, 2018 at 4:43 PM, Fāng-ruì Sòng 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-sc

Re: r338291 - Remove trailing space

2018-07-30 Thread Aaron Ballman via cfe-commits
On Mon, Jul 30, 2018 at 5:36 PM, Michael Kruse wrote: > Can you point me to such a discussion about trailing whitespace? I don't know of one, which is why I am concerned with these commits. > It seems to contradict the conclusion in another current thread to not > consider churn for out-of-tree

Re: r338291 - Remove trailing space

2018-07-30 Thread Aaron Ballman via cfe-commits
On Mon, Jul 30, 2018 at 4:43 PM, Fāng-ruì Sòng 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 cons

Re: r338291 - Remove trailing space

2018-07-30 Thread Michael Kruse via cfe-commits
Can you point me to such a discussion about trailing whitespace? It seems to contradict the conclusion in another current thread to not consider churn for out-of-tree users (for C++ API in this case) https://lists.llvm.org/pipermail/cfe-dev/2018-July/058579.html > We have historically taken the

Re: r338291 - Remove trailing space

2018-07-30 Thread Fāng-ruì Sòng via cfe-commits
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 ey

Re: r338291 - Remove trailing space

2018-07-30 Thread Fāng-ruì Sòng via cfe-commits
Does this apply to only public headers (include/llvm include/llvm-c include/clang ...) or everything? (lib/**/*.{cpp,h})? On Mon, Jul 30, 2018 at 1:36 PM Aaron Ballman wrote: > > On Mon, Jul 30, 2018 at 4:24 PM, Michael Kruse > wrote: > > I think removing trailing space is a good thing. Some edit

Re: r338291 - Remove trailing space

2018-07-30 Thread Aaron Ballman via cfe-commits
On Mon, Jul 30, 2018 at 4:24 PM, Michael Kruse wrote: > I think removing trailing space is a good thing. Some editors remove > any trailing space when saving a file. This shows up in diffs that I > then have to undo manually. I've also run into the same issues on occasion. However, this isn't abo

Re: r338291 - Remove trailing space

2018-07-30 Thread Aaron Ballman via cfe-commits
On Mon, Jul 30, 2018 at 3:52 PM, Fāng-ruì Sòng wrote: > Oops.. sorry but now they have been committed.. The commits can still be reverted and I think they should be. ~Aaron ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

Re: r338291 - Remove trailing space

2018-07-30 Thread Fāng-ruì Sòng via cfe-commits
Oops.. sorry but now they have been committed.. On Mon, Jul 30, 2018 at 12:31 PM Aaron Ballman wrote: > > This is an extraordinary amount of churn for very little value, IMO. > The same goes for r338291. Were these changes discussed somewhere > before being committed? I worry about the negative im