Messages in this thread | | | Date | Sat, 27 Sep 2008 22:00:28 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH v9] Unified trace buffer |
| |
* Steven Rostedt <rostedt@goodmis.org> wrote:
> > 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.
that's really not a hard limit, but yeah.
generally, with clean and simple functions it's easy to keep it.
> > 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); > }
yeah, sounds sane.
> > 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.
this:
> 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 > */
or:
> /* > * Left over page padding. 'array' is ignored, > * 'size' is variable depending on how much padding is needed. > */ > RB_TYPE_PADDING, > > /* > * Extent the time delta, > * array[0] = time delta (28 .. 59), size = 8 bytes > */ > RB_TYPE_TIME_EXTENT,
oh, btw., that's a spelling mistake: s/extend/extend ?
Ingo
| |