Messages in this thread | | | Date | Sat, 27 Sep 2008 15:54:07 -0400 (EDT) | From | Steven Rostedt <> | Subject | Re: [PATCH v9] Unified trace buffer |
| |
On Sat, 27 Sep 2008, Ingo Molnar wrote: > > * 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.
> > > > 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); }
> 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
| |