I think jaggedness in the example you provide comes mainly from the fact that the second comment has multiple logical blocks. I have formatted both comments at 70 and at 80, here is the outcome: http://pastebin.com/nRQB0nCD
While the first comment indeed looks better when wrapped at 70, I can't say the same about the second one. I would say, that the longer a line could be, the less jagged the comment block is. The ratio (`averageWordLength` / `maxLineLength`) approaches 0 as `maxLineLenght` approaches infinity, which means wrapping a long word right before the line end should be perceived less jagged : ). Also, the longer an individual line can be, the less total lines are needed for a comment block, which reduces jaggedness and makes code a little bit more readable. But my strongest argument is that having a separate soft rule for comments is hard to enforce. I think what we can do is to encourage contributors / committers to wrap comments in the most logical way—like the first comment in the example you provide—even if the line length is not fully utilized. Having said that, I would rather keep a single number: hard limit at 80 for simplicity. On Thu, Nov 5, 2015 at 10:15 PM, Benjamin Mahler <benjamin.mah...@gmail.com> wrote: > This has come up in a couple of reviews, seems like we should add some soft > guidelines around how to format comments for readability. > > In particular, the reason that we wrapped at 70 in the past was for > readability, so it would be great to continue doing so as a soft stylistic > rule. The other thing we've been doing for readability is reducing > "jaggedness" (variability in line lengths). > > It would be great to establish these as soft rules and encourage new > contributors / committers to follow them. Compare these two comments in > Master::updateTask. The first one wraps at 70 and reduces jagedness, the > second wraps at 80 and is more jagged: > > https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6057 > https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6072 > > I can provide more examples to help clarify. If no one objects, I'll follow > up with an update to the style guide. Thoughts appreciated! > > On Thu, Sep 10, 2015 at 8:59 AM, Bernd Mathiske <be...@mesosphere.io> > wrote: > > > +1 > > > On Sep 10, 2015, at 4:21 PM, tommy xiao <xia...@gmail.com> wrote: > > > > > > +1 > > > > > > 2015-09-10 9:44 GMT+08:00 Marco Massenzio <ma...@mesosphere.io>: > > > > > >> +1 > > >> > > >> > > >> > > >> > > >> Thanks, Michael! > > >> > > >> > > >> > > >> — > > >> Sent from my iPhone, which is not as good as you'd hope to fix trypos > n > > >> abbrvtn. > > >> > > >> On Wed, Sep 9, 2015 at 6:23 PM, Michael Park <mcyp...@gmail.com> > wrote: > > >> > > >>> I've removed the 70 column restriction on comments from the style > > guide: > > >>> > > >> > > > https://github.com/apache/mesos/commit/f9c2604ea97b91f8a9ec3b2863317761679b1c86 > > >>> Also, based on the comments, it seems like we should allow 80 column > > >>> comments but omit the sweeping change. > > >>> Thanks, > > >>> MPark. > > >>> On Wed, Aug 12, 2015 at 6:13 PM Marco Massenzio <ma...@mesosphere.io > > > > >> wrote: > > >>>> On Wed, Aug 12, 2015 at 4:09 AM, Bernd Mathiske < > be...@mesosphere.io> > > >>>> wrote: > > >>>> > > >>>>> Like BenM, > > >>>>> > > >>>>> +1 on allowing 80 column comments > > >>>>> > > >>>> +1 > > >>>> (it really IS annoying having to keep an eye on the bottom column > > >> counter > > >>>> when typing comments :) > > >>>> > > >>>> > > >>>>> -1 on sweeping changes; incremental changes when touching old > > comments > > >>>>> will do IMHO > > >>>>> > > >>>>> +1 on the -1? :) > > >>>> Incremental changes are good and I doubt anyone will be "confused" > by > > >> them. > > >>>> > > >>>> > > >>>>> Bernd > > >>>>> > > >>>>>> On Aug 12, 2015, at 12:51 AM, Michael Park <mcyp...@gmail.com> > > >> wrote: > > >>>>>> > > >>>>>> Ben, thanks for your input! > > >>>>>> > > >>>>>> Another update on this topic: the patches around break before > braces > > >>>> for > > >>>>>> *enum* style and overloaded operators have been committed. > > >>>>>> > > >>>>>> On Tue, Aug 11, 2015 at 6:23 PM Benjamin Mahler < > > >>>>> benjamin.mah...@gmail.com> > > >>>>>> wrote: > > >>>>>> > > >>>>>>> We already don't necessarily wrap at 70 characters (often we wrap > > >>>>> before 70 > > >>>>>>> to reduce "jaggedness" or to make it look cleaner). > > >>>>>>> > > >>>>>>> So with the change to 80, this still makes all existing comments > > >>>> valid. > > >>>>> We > > >>>>>>> can still encourage folks to write paragraphs in a way that is > > >> easy to > > >>>>>>> digest for the reader. That is, I think we should still be trying > > >> not > > >>>> to > > >>>>>>> write jagged paragraphs of comments, it's just not a hard > stylistic > > >>>>>>> violation given we don't have an algorithm for this. > > >>>>>>> > > >>>>>>> So +1 to relaxing the hard 70 character rule, but -1 to sweeping > > >>>> across > > >>>>> all > > >>>>>>> the comments or doing wrapping based only on line length rather > > >> than > > >>>>>>> jaggedness going forward. > > >>>>>>> > > >>>>>>> On Sat, Aug 8, 2015 at 3:25 PM, Joris Van Remoortere < > > >>>>> jo...@mesosphere.io> > > >>>>>>> wrote: > > >>>>>>> > > >>>>>>>> I will volunteer to update all the comments to wrap at 80 if we > > >> agree > > >>>>> to > > >>>>>>>> keep the code base consistent. > > >>>>>>>> Naturally that is also my vote ;-) > > >>>>>>>> Joris > > >>>>>>>> > > >>>>>>>>> On Aug 8, 2015, at 1:40 PM, Michael Park <mcyp...@gmail.com> > > >> wrote: > > >>>>>>>>> > > >>>>>>>>> An update on this topic since we covered it at the community > > >>>> developer > > >>>>>>>> sync. > > >>>>>>>>> > > >>>>>>>>> 1. We will adopt *Mozilla*'s *BreakBeforeBraces* style as their > > >>>> style > > >>>>>>>> is > > >>>>>>>>> equivalent to ours. The only change this entails for our > > >> codebase > > >>>> is > > >>>>>>> to > > >>>>>>>>> consistently wrap the braces for *enum* definitions, as we're > > >>>>>>> currently > > >>>>>>>>> inconsistent. I've taken on the work involved in this change: > > >>>>>>>>> - stout: https://reviews.apache.org/r/37258 > > >>>>>>>>> - libprocess: https://reviews.apache.org/r/37259 > > >>>>>>>>> - mesos: https://reviews.apache.org/r/37260 > > >>>>>>>>> 2. We will drop the rule for adding spaces around overloaded > > >>>>>>>>> operators. We'll simply do a sweep of the codebase to update > > >> all of > > >>>>>>>> them > > >>>>>>>>> consistently. Artem has kindly taken action on this already: > > >>>>>>>>> - stout: https://reviews.apache.org/r/37018/ > > >>>>>>>>> - libprocess: https://reviews.apache.org/r/37017/ > > >>>>>>>>> - mesos: https://reviews.apache.org/r/37013/ > > >>>>>>>>> 3. We will drop the rule for wrapping comments at 70 > > >> characters. > > >>>>>>> We > > >>>>>>>>> have a few options to proceed here: > > >>>>>>>>> - Keep all the existing comments in tact, and simply allow > > >> new > > >>>>>>>>> comments to wrap at 80, this is less work. > > >>>>>>>>> - Update all instances of the comments wrapping at 70 to be > > >>>>>>> wrapped > > >>>>>>>>> at 80, so that we can be consistent. > > >>>>>>>>> > > >>>>>>>>> I proposed that we simply allow new comments to wrap at 80, > but I > > >>>> have > > >>>>>>>>> heard arguments to update the existing comments, so that we can > > >> be > > >>>>>>>>> consistent across the codebase. If you have a > suggestion/opinion > > >> on > > >>>>> how > > >>>>>>>> we > > >>>>>>>>> should proceed with (3), please share! > > >>>>>>>>> > > >>>>>>>>> Thanks, > > >>>>>>>>> > > >>>>>>>>> MPark. > > >>>>>>>>> > > >>>>>>>>> On Mon, Aug 3, 2015 at 2:01 PM Alexander Rojas < > > >>>>>>> alexan...@mesosphere.io> > > >>>>>>>>> wrote: > > >>>>>>>>> > > >>>>>>>>>> I also vote up for that! I rather change our guidelines a > little > > >>>> bit > > >>>>>>>> than > > >>>>>>>>>> waiting for months > > >>>>>>>>>> to get our changes into the clang-format source without any > > >>>> security > > >>>>>>>> that > > >>>>>>>>>> it will actually land. > > >>>>>>>>>> > > >>>>>>>>>>> On 31 Jul 2015, at 09:31, Alex Rukletsov < > a...@mesosphere.com> > > >>>>>>> wrote: > > >>>>>>>>>>> > > >>>>>>>>>>> I think automation is very important. If we should slightly > > >> change > > >>>>>>> our > > >>>>>>>>>>> style in order to set-up easily enforceable rules, I vote > with > > >>>> both > > >>>>>>>> hands > > >>>>>>>>>>> for that. > > >>>>>>>>>>> > > >>>>>>>>>>>> On Fri, Jul 31, 2015 at 3:25 AM, Michael Park < > > >> mcyp...@gmail.com > > >>>>> > > >>>>>>>> wrote: > > >>>>>>>>>>>> > > >>>>>>>>>>>> Oops, sorry I was so excited that this could just solve the > > >> issue > > >>>>>>>> that I > > >>>>>>>>>>>> forgot to answer your question. > > >>>>>>>>>>>> > > >>>>>>>>>>>> In general, the clang-format strives to adopt widely used > > >> styles, > > >>>>>>>> which > > >>>>>>>>>> I'm > > >>>>>>>>>>>> not sure if we would be considered widely used. Aside from > > >> that, > > >>>>>>>> another > > >>>>>>>>>>>> concern was that it could take a while for our style > > >> proposals to > > >>>>>>> make > > >>>>>>>>>> it > > >>>>>>>>>>>> upstream and for it to be useful. > > >>>>>>>>>>>> > > >>>>>>>>>>>>> On Thu, Jul 30, 2015, 6:12 PM Michael Park < > > >> mcyp...@gmail.com> > > >>>>>>>> wrote: > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> Is it worth adding our own style? > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> I noticed other have (LLVM, Google, Chromium, Mozilla, > > >> WebKit.). > > >>>>>>> How > > >>>>>>>>>>>>>> hard is it? > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> I was just looking into this again and *Mozilla* was added > as > > >>>> the > > >>>>>>>>>> newest > > >>>>>>>>>>>>> *BreakBeforeBraces* style. It breaks before braces on enum, > > >>>>>>> function, > > >>>>>>>>>> and > > >>>>>>>>>>>>> record definitions (struct, class, union). I think we can > > >>>> actually > > >>>>>>>> use > > >>>>>>>>>>>> that > > >>>>>>>>>>>>> one and be done with it. Having looked through the > codebase, > > >> we > > >>>>>>> wrap > > >>>>>>>>>> the > > >>>>>>>>>>>>> braces for *enum* for about half of the cases. It would be > > >> about > > >>>>> 35 > > >>>>>>>>>>>>> instances that we have to fix from what I can see in our > > >>>> codebase. > > >>>>>>>> What > > >>>>>>>>>>>> do > > >>>>>>>>>>>>> you think? > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> On Thu, Jul 30, 2015 at 5:14 PM Benjamin Mahler < > > >>>>>>>>>>>> benjamin.mah...@gmail.com> > > >>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>> > > >>>>>>>>>>>>>> Is it worth adding our own style? > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> I noticed other have (LLVM, Google, Chromium, Mozilla, > > >>>> WebKit.). > > >>>>>>> How > > >>>>>>>>>>>> hard > > >>>>>>>>>>>>>> is it? > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> On Thu, Jul 30, 2015 at 4:23 PM, Michael Park < > > >>>> mcyp...@gmail.com > > >>>>>> > > >>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> There are a few syntactical Mesos style guidelines that I > > >>>> would > > >>>>>>>> like > > >>>>>>>>>>>> to > > >>>>>>>>>>>>>>> propose to drop. They are: > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> 1. Open braces for namespace should not be wrapped. > > >>>>>>>>>>>>>>> *NOTE*: The Google style guide does not wrap the brace > > >> after > > >>>>>>>>>>>>>>> *namespace*, > > >>>>>>>>>>>>>>> and the Mesos style guide does not mention a rule at all. > > >> But > > >>>> it > > >>>>>>>> is > > >>>>>>>>>>>>>>> consistent throughout the codebase. > > >>>>>>>>>>>>>>> 2. Overloaded operators should be padded with spaces. > > >>>>>>>>>>>>>>> 3. Comments should be wrapped at 70 characters. > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> The main motivation is that as a community we would like > to > > >>>>>>> reduce > > >>>>>>>>>> the > > >>>>>>>>>>>>>>> discrepancy between what *clang-format* produces. This > is a > > >>>> dual > > >>>>>>>>>>>>>> effort, as > > >>>>>>>>>>>>>>> we work on improving *clang-format* to support some of > our > > >>>> style > > >>>>>>>>>> which > > >>>>>>>>>>>>>> is > > >>>>>>>>>>>>>>> popular in the C++ community as well. Wrapping the > function > > >>>>>>>> arguments > > >>>>>>>>>>>> to > > >>>>>>>>>>>>>>> avoid "jaggedness" for example is a feature request which > > >> is > > >>>>>>> being > > >>>>>>>>>>>>>> tracked > > >>>>>>>>>>>>>>> at: https://llvm.org/bugs/show_bug.cgi?id=23422 > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> Going forward, the proposal is to update all of the > > >> instances > > >>>> of > > >>>>>>>> (1) > > >>>>>>>>>>>> and > > >>>>>>>>>>>>>>> (2) at once. For (3), we can simply relax the constraint > > >> from > > >>>> 70 > > >>>>>>> to > > >>>>>>>>>> 80 > > >>>>>>>>>>>>>>> without touching the existing comments. > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> Does anyone have any strong opinions about dropping any > of > > >>>> the 3 > > >>>>>>>>>> rules > > >>>>>>>>>>>>>>> above? > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> Thanks, > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> MPark. > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>> > > >>>>>>> > > >>>>> > > >>>>> > > >>>> > > >> > > > > > > > > > > > > -- > > > Deshi Xiao > > > Twitter: xds2000 > > > E-mail: xiaods(AT)gmail.com > > > > >
