On 12/20/2013 11:18 PM, Rafael Espíndola wrote:> On 20 December 2013 15:52, reed kotler <rkotler at mips.com> wrote: >> We are considering running clang format on the whole Mips target. >> >> Is there any rule against this? > > I don't think there is a rule, but even simpler things like "remove > all trailing spaces" were not common in the past > >> Is there any good argument against doing this even if there is no rule >> against it? > > It think the argument was that it makes history harder to follow. A > git blame lands you on the reformatting patch and you have to manually > skip them. > > I don't personally consider that a strong argument and I think polly > did just that (run clang-format on everything), so it might be OK > these days.Unfortunately for Polly it was not as clean as we started using clang-format early, so we had to format piece by piece the sections that clang-format could already handle. However, we now have an automatic clang-format buildbot so formatting became (in some way) a 'solved problem'. One benefit of fully formatted source code is that there is no need for any new patches to include 'formatting fixes', which previously sometimes obfuscated submitted patches and commits. In fact, patch reviews are now free of formatting discussions. If the patch passes make polly-check-format, it is ready to be discussed. (If it does not, make polly-update-format needs to be run). Also, having a single formatting commit to skip might arguably be easier, than to have a mixed history of formatting and patches. One reason that may hold us back from formatting all LLVM code is the risk of clang-format changing its formatting behavior. The only thing worse than formatting all code is formatting all code twice. ;) From my experience with Polly, clang-format's formatting has been largely stable the last months. Changes in its formatting behavior have normally been fixed quickly. So it may make sense to format more code. I would still be afraid of formatting all LLVM, but maybe a backend is a reasonable sized chunk. Maybe Manual and Daniel have some experience in formatting a larger set of files. Cheers, Tobias
Thanks Rafael and Tobias for the feedback. It's particularly good to hear that clang-format has helped Polly eliminate formatting discussions from patch reviews since this is one of the key benefits we hope to gain from doing this. The distribution of our team across different time zones makes these discussions rather slow.> One reason that may hold us back from formatting all LLVM code is the risk of > clang-format changing its formatting behavior. The only thing worse than > formatting all code is formatting all code twice. ;)That's a reasonable concern both for all LLVM code and for a single backend. I think that the benefits of mass-formatting outweigh the costs as long as we rarely need to do it. However, doing it frequently becomes more of a hindrance than it's worth. It seems unlikely that clang-formats behaviour will change significantly (excluding occasional bugs). For example, indentation style and depth should remain constant. As long as any behaviour changes are fairly minor (such as the precise point a line break is inserted for >80 col lines), then it should be ok to do one mass-format and then maintain the good formatting by formatting only the lines touched by each patch. Eventually the patches might get noisy enough to warrant another mass-format but I would hope that it would take upwards of 5 years to reach the point where it becomes a sufficient nuisance again.> -----Original Message----- > From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] > On Behalf Of Tobias Grosser > Sent: 20 December 2013 23:00 > To: Rafael Espíndola; Reed Kotler > Cc: LLVMdev at cs.uiuc.edu > Subject: Re: [LLVMdev] running clang format on the Mips target > > On 12/20/2013 11:18 PM, Rafael Espíndola wrote: > > On 20 December 2013 15:52, reed kotler <rkotler at mips.com> wrote: > >> We are considering running clang format on the whole Mips target. > >> > >> Is there any rule against this? > > > > I don't think there is a rule, but even simpler things like "remove > > all trailing spaces" were not common in the past > > > >> Is there any good argument against doing this even if there is no > >> rule against it? > > > > It think the argument was that it makes history harder to follow. A > > git blame lands you on the reformatting patch and you have to manually > > skip them. > > > > I don't personally consider that a strong argument and I think polly > > did just that (run clang-format on everything), so it might be OK > > these days. > > Unfortunately for Polly it was not as clean as we started using clang-format > early, so we had to format piece by piece the sections that clang-format > could already handle. > > However, we now have an automatic clang-format buildbot so formatting > became (in some way) a 'solved problem'. > > One benefit of fully formatted source code is that there is no need for any > new patches to include 'formatting fixes', which previously sometimes > obfuscated submitted patches and commits. In fact, patch reviews are now > free of formatting discussions. If the patch passes make polly-check-format, > it is ready to be discussed. (If it does not, make polly-update-format needs to > be run). Also, having a single formatting commit to skip might arguably be > easier, than to have a mixed history of formatting and patches. > > One reason that may hold us back from formatting all LLVM code is the risk of > clang-format changing its formatting behavior. The only thing worse than > formatting all code is formatting all code twice. ;) > > From my experience with Polly, clang-format's formatting has been largely > stable the last months. Changes in its formatting behavior have normally > been fixed quickly. So it may make sense to format more code. > I would still be afraid of formatting all LLVM, but maybe a backend is a > reasonable sized chunk. > > Maybe Manual and Daniel have some experience in formatting a larger set of > files. > > Cheers, > Tobias > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Manuel, Daniel, I forgot to CC you in this mail. The thread here may be interesting to you, also in the light of the last discussion we had. Tobias On 12/21/2013 12:00 AM, Tobias Grosser wrote:> On 12/20/2013 11:18 PM, Rafael Espíndola wrote: >> On 20 December 2013 15:52, reed kotler <rkotler at mips.com> wrote: >>> We are considering running clang format on the whole Mips target. >>> >>> Is there any rule against this? >> >> I don't think there is a rule, but even simpler things like "remove >> all trailing spaces" were not common in the past >> >>> Is there any good argument against doing this even if there is no rule >>> against it? >> >> It think the argument was that it makes history harder to follow. A >> git blame lands you on the reformatting patch and you have to manually >> skip them. >> >> I don't personally consider that a strong argument and I think polly >> did just that (run clang-format on everything), so it might be OK >> these days. > > Unfortunately for Polly it was not as clean as we started using > clang-format early, so we had to format piece by piece the sections that > clang-format could already handle. > > However, we now have an automatic clang-format buildbot so formatting > became (in some way) a 'solved problem'. > > One benefit of fully formatted source code is that there is no need for > any new patches to include 'formatting fixes', which previously > sometimes obfuscated submitted patches and commits. In fact, patch > reviews are now free of formatting discussions. If the patch passes > make polly-check-format, it is ready to be discussed. (If it does not, > make polly-update-format needs to be run). Also, having a single > formatting commit to skip might arguably be easier, than to have a mixed > history of formatting and patches. > > One reason that may hold us back from formatting all LLVM code is the > risk of clang-format changing its formatting behavior. The only thing > worse than formatting all code is formatting all code twice. ;) > > From my experience with Polly, clang-format's formatting has been > largely stable the last months. Changes in its formatting behavior have > normally been fixed quickly. So it may make sense to format more code. > I would still be afraid of formatting all LLVM, but maybe a backend is > a reasonable sized chunk. > > Maybe Manual and Daniel have some experience in formatting a larger set > of files.
FYI, our usual recommendation is to use clang-format only on changed code, unless there's strong evidence that the cost of the initial reformatting is basically zero (which usually means a team / community is pretty unanimous about the decision). On Thu, Jan 9, 2014 at 11:53 AM, Tobias Grosser <tobias at grosser.es> wrote:> Manuel, Daniel, > > I forgot to CC you in this mail. The thread here may be interesting to > you, also in the light of the last discussion we had. > > Tobias > > > On 12/21/2013 12:00 AM, Tobias Grosser wrote: > >> On 12/20/2013 11:18 PM, Rafael Espíndola wrote: >> >>> On 20 December 2013 15:52, reed kotler <rkotler at mips.com> wrote: >>> >>>> We are considering running clang format on the whole Mips target. >>>> >>>> Is there any rule against this? >>>> >>> >>> I don't think there is a rule, but even simpler things like "remove >>> all trailing spaces" were not common in the past >>> >>> Is there any good argument against doing this even if there is no rule >>>> against it? >>>> >>> >>> It think the argument was that it makes history harder to follow. A >>> git blame lands you on the reformatting patch and you have to manually >>> skip them. >>> >>> I don't personally consider that a strong argument and I think polly >>> did just that (run clang-format on everything), so it might be OK >>> these days. >>> >> >> Unfortunately for Polly it was not as clean as we started using >> clang-format early, so we had to format piece by piece the sections that >> clang-format could already handle. >> >> However, we now have an automatic clang-format buildbot so formatting >> became (in some way) a 'solved problem'. >> >> One benefit of fully formatted source code is that there is no need for >> any new patches to include 'formatting fixes', which previously >> sometimes obfuscated submitted patches and commits. In fact, patch >> reviews are now free of formatting discussions. If the patch passes >> make polly-check-format, it is ready to be discussed. (If it does not, >> make polly-update-format needs to be run). Also, having a single >> formatting commit to skip might arguably be easier, than to have a mixed >> history of formatting and patches. >> >> One reason that may hold us back from formatting all LLVM code is the >> risk of clang-format changing its formatting behavior. The only thing >> worse than formatting all code is formatting all code twice. ;) >> >> From my experience with Polly, clang-format's formatting has been >> largely stable the last months. Changes in its formatting behavior have >> normally been fixed quickly. So it may make sense to format more code. >> I would still be afraid of formatting all LLVM, but maybe a backend is >> a reasonable sized chunk. >> >> Maybe Manual and Daniel have some experience in formatting a larger set >> of files. >> > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140109/123b7d5e/attachment.html>