On Sat, 27 Sep 2008, Ingo Molnar wrote:
quoted text
> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> that's even worse i think :-/ And this isnt bikeshed-painting really, 
> the RNGBF_ name hurts my eyes and RB_ is definitely confusing to read. 
> (as the rbtree constants are in capitals as well and similarly named)
> 
>  RING_TYPE_PADDING
> 
> or:
> 
>  RINGBUF_TYPE_PADDING
> 
> yes, it's longer, but still, saner.

I don't mind the extra typing, it is just a bit more difficult to keep in
the 80 character line limit.

quoted text
> 
> > > too large, please uninline.
> > 
> > I calculated this on x86_64 to add 78 bytes. Is that still too big?
> 
> yes, way too big. Sometimes we make savings from a 10 bytes function 
> already. (but it's always case dependent - if a function has a lot of 
> parameters then uninlining can hurt)
> 
> the only exception would be if there's normally only a single 
> instantiation per tracer, and if it's in the absolute tracing hotpath. 

It is a hot path in the internals. Perhaps I'll make an inline function
in the interal code "rb_event_length" and have the other users call.

unsigned ring_buffer_event(struct ring_buffer_event *event)
{
	return rb_event_length(event);
}

quoted text
> no, it is not readable. My point was that you should do:
> > 
> >    RB_ENUM_TYPE,	/*
> > 			 * Comment
> > 			 */
> > 
> > The comment is not at the same line as the enum, which also looks 
> > unpleasing.
> 
> but you did:
> 
> >    RB_ENUM_TYPE,	/* Comment
> > 			 */
> 
> So i suggested to fix it to:
> 
>  +	RB_TYPE_TIME_EXTENT,	/*
>  +				 * Extent the time delta
>  +				 * array[0] = time delta (28 .. 59)
>  +				 * size = 8 bytes
>  +				 */
> 
> ok? I.e. "comment" should have the same visual properties as other 
> comments.
> 
> I fully agree with moving it next to the enum, i sometimes use that 
> style too, it's a nice touch and more readable in this case than 
> comment-ahead. (which we use for statements)

But then we have:

        RB_TYPE_PADDING,        /*
				 * Left over page padding
                                 * array is ignored
                                 * size is variable depending on
                                 * how much padding is needed
                                 */
        RB_TYPE_TIME_EXTENT,    /*
				 * Extent the time delta
                                 * array[0] = time delta (28 .. 59)
                                 * size = 8 bytes
                                 */

Where it is not as easy to see which comment is with which enum. 
Especially when you have many enums. That's why I like the method I used 
with:

        RB_TYPE_PADDING,        /* Left over page padding
                                 * array is ignored
                                 * size is variable depending on
                                 * how much padding is needed
                                 */
        RB_TYPE_TIME_EXTENT,    /* Extent the time delta
                                 * array[0] = time delta (28 .. 59)
                                 * size = 8 bytes
                                 */

Where it is very easy to notice which comment goes with which enum.

-- Steve

--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/