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>
I would slightly alter this recommendation depending on whether the codebase is currently in a reasonably consistent state (both internally and wrt. what clang-format would do). The basic assumption is that code is easier to read if it is more consistent. If the code base is currently inconsistently formatted, then a full reformatting might provide benefit. If it is significantly different from clang-format, then an incremental approach has the cost of making the code more inconsistent (at least temporarily). On Thu, Jan 9, 2014 at 12:03 PM, Manuel Klimek <klimek at google.com> wrote:> 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/1f103094/attachment.html>
Thanks for the recommendations. The MIPS team itself is unanimously agreed that an initial formatting is worth the cost. I've had an objection from David Chisnall which I'm trying to accommodate by waiting for him to upstream the relevant parts of his out-of-tree modifications before going ahead. I haven't had any other objections but I intend to send an email giving three weeks' notice calling for comments/objections in case there are others. At the moment, the MIPS backend is generally consistent inside each file but not between them. When I ran clang-format on lib/Target/Mips/*.{cpp,h} in my working copy, the diff was 555KB, so I'd say it's generally inconsistent with clang-format. From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] On Behalf Of Daniel Jasper Sent: 09 January 2014 11:19 To: Manuel Klimek Cc: Reed Kotler; Tobias Grosser; LLVMdev at cs.uiuc.edu Subject: Re: [LLVMdev] running clang format on the Mips target I would slightly alter this recommendation depending on whether the codebase is currently in a reasonably consistent state (both internally and wrt. what clang-format would do). The basic assumption is that code is easier to read if it is more consistent. If the code base is currently inconsistently formatted, then a full reformatting might provide benefit. If it is significantly different from clang-format, then an incremental approach has the cost of making the code more inconsistent (at least temporarily). On Thu, Jan 9, 2014 at 12:03 PM, Manuel Klimek <klimek at google.com<mailto:klimek at google.com>> wrote: 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<mailto: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<mailto: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/20140113/9f7646f6/attachment.html>