On Fri, 15 Jun 2018 10:03:43 -0600 Mathieu Poirier mathieu.poirier@linaro.org wrote:
On Thu, 14 Jun 2018 at 17:53, Kim Phillips kim.phillips@arm.com wrote:
On Thu, 14 Jun 2018 13:41:33 -0600 Mathieu Poirier mathieu.poirier@linaro.org wrote:
@@ -430,24 +420,30 @@ static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm, struct auxtrace_queue *queue, unsigned int queue_nr) {
int ret = 0; struct cs_etm_queue *etmq = queue->priv; if (list_empty(&queue->head) || etmq)
return 0;
goto out;
What does 'goto out' buy us here? Having the return in-place is easier/clearer to read.
It streamlines the exit path.
How? It unnecessarily redirects it and sends the reader hunting.
This is how the function body looks after this patch:
{ int ret = 0; struct cs_etm_queue *etmq = queue->priv;
if (list_empty(&queue->head) || etmq) goto out;
etmq = cs_etm__alloc_queue(etm);
if (!etmq) { ret = -ENOMEM; goto out; }
queue->priv = etmq; etmq->etm = etm; etmq->queue_nr = queue_nr; etmq->cpu = queue->cpu; etmq->tid = queue->tid; etmq->pid = -1; etmq->offset = 0; etmq->period_instructions = 0;
out: return ret; }
And this is how it would look whilst continuing to directly return from either of the exit paths:
{ struct cs_etm_queue *etmq = queue->priv;
if (list_empty(&queue->head) || etmq) return 0;
etmq = cs_etm__alloc_queue(etm); if (!etmq) return -ENOMEM;
queue->priv = etmq; etmq->etm = etm; etmq->queue_nr = queue_nr; etmq->cpu = queue->cpu; etmq->tid = queue->tid; etmq->pid = -1; etmq->offset = 0; etmq->period_instructions = 0;
return 0; }
The latter is easier to follow, and doesn't need a 'ret' variable, any gotos, or any labels at all.
etmq = cs_etm__alloc_queue(etm, queue_nr);
etmq = cs_etm__alloc_queue(etm);
if (!etmq)
return -ENOMEM;
if (!etmq) {
ret = -ENOMEM;
goto out;
}
same here.
queue->priv = etmq;
if (queue->cpu != -1)
etmq->cpu = queue->cpu;
etmq->etm = etm;
etmq->queue_nr = queue_nr;
etmq->cpu = queue->cpu;
the != -1 condition above is being removed and not replaced, yet there's no explanation in the commit text. Should it be a part of a different patch?
Variable queue->cpu is either -1 (per-thread) or hold the value of a CPU (CPU-wide). So instead of invariably setting etmq->cpu to -1 in cs_etm__alloc_queue() and checking if it is worth changing that in cs_etm__setup_queue() as the current code does, directly doing etmq->cpu = queue->cpu yields exactly the same result.
OK, it would be nice to have that explanation in the commit text.
Thanks,
Kim