Re: [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area

[Nikita Pettik <korablev@tarantool.org>]

On 23 Feb 18:43, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch, welcome to the first review iteration!
> 
> Finally something good, not related to SQL/replication/application server
> modules.
> 
> > +/**
> > + * Add a new error to the diagnostics area. It is added to the
> > + * tail, so that list forms stack.
> > + * @param diag Diagnostics area.
> > + * @param e Error to be added.
> > + */
> > +static inline void
> > +diag_add_error(struct diag *diag, struct error *e)
> > +{
> > +	assert(e != NULL);
> > +	error_ref(e);
> > +	error_unlink_tail(e);
> > +	e->next = diag->last;
> 
> 15. I thought, that 'next' member is responsible for keeping a
> reference, but since you don't ref 'diag->last' here, I am not
> sure now. Could you please clarify?

Yes, but since diag->last is not longer the head of diag list,
it also should be unrefed. One ref and one unref - as a result
no ref actions. 
 
> > +	if (diag->last != NULL)
> > +		diag->last->prev = e;
> >  	diag->last = e;
> >  }
> >  
> 
> 21. It is not really necessary to use assertion here. It will fail
> without assertion too in case of a problem. Assertion is needed,
> when you want an exception. For example, when you call a function,
> inside which you do tests.

IMHO assertions underline the fact of testing result of particular
action (since we don't have explicit test case borders, it may turn
out to be vital while attempting at exploring test).
We can discuss it in server chat with other members, I do not mind both
approaches.
 
> Instead of an assertion it is better to write something like that:
> 
>     e3.prev == e2 or {e3, e3.prev, e2}
> 
> In that case, if the == is false, you will see not only a false in
> .reject file, but also actual values of e3, e3.prev, and e2. This
> is especially helpful, when error is not stable, and in Travis/Gitlab,
> because you see values right in the web console.

These tests are quite easy to reproduce copying them to Tarantool's
console.