Re: [PATCH 05/18] perf tools: Add ordered_events_(get|put) interface

[Arnaldo Carvalho de Melo]

Em Sun, Jun 29, 2014 at 10:50:24AM -0600, David Ahern escreveu:
> On 6/29/14, 10:39 AM, Jiri Olsa wrote:
> >On Fri, Jun 27, 2014 at 05:06:36PM -0600, David Ahern wrote:
> >>On 6/18/14, 8:58 AM, Jiri Olsa wrote:
> >>>+static struct ordered_event*
> >>>+ordered_events_get(struct ordered_events_queue *q, u64 timestamp)
> >>>+{
> >>>+	struct ordered_event *new;
> >>>+
> >>>+	new = alloc_event(q);
> >>>+	if (new) {
> >>>+		new->timestamp = timestamp;
> >>>+		queue_event(q, new);
> >>>+	}
> >>>+
> >>>+	return new;
> >>>+}

> >>The _get name does not really correlate with what is happening -- ie.,
> >>allocate a new event and add it to the queue. There is no reference taken
> >>either.

> >ook.. so how about ordered_events_alloc ordered_events_free

ordered_events__new() and ordered_events__delete(), to be consistent
with general naming for constructors and destructors in tools/perf/ :-)

I would also not use "new" as the name of the new instance, as above,
but would rather use 'oe', shortcut for ordered event, or even oevent,
as elsewhere suggested in this thread.

> >>>+static void
> >>>+ordered_event_put(struct ordered_events_queue *q, struct ordered_event *iter)
> >>>+{
> >>>+	list_del(&iter->list);
> >>>+	list_add(&iter->list, &q->cache);
> >>>+	q->nr_events--;
> >>>+}
> >>
> >>Similarly here with the _put. In this case the function is moving the event
> >>from one list to another. And how about something else for the name besides
> >>iter -- oe, or oevent?
> >
> >how about 'event' ?
> 
> Already a heavily used keyword in perf, that's why I was thinking oe or
> oevent -- besides it is a struct ordered_event not an event.

Agreed on all counts.
 
> The bigger thing to me with this patch is the _get/_put names.

Agreed, those are for reference counting.

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