From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 514FD441841 for ; Wed, 25 Mar 2020 04:34:03 +0300 (MSK) Date: Wed, 25 Mar 2020 01:34:02 +0000 From: Nikita Pettik Message-ID: <20200325013402.GB30598@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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.