<div dir="ltr"><span style="line-height:1.5">> This is the standard error checking model in C-based codes. W</span><span style="line-height:1.5">hy do you </span><span style="line-height:1.5">call this clutter ?</span><br><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">Because of</span></div><div><span style="line-height:1.5">```</span></div><div><span style="line-height:1.5">const int a = giveMeFive();</span></div><div><span style="line-height:1.5">```</span></div><div><span style="line-height:1.5">vs.</span></div><div><span style="line-height:1.5">```</span></div><div><span style="line-height:1.5">int a;</span></div><div><span style="line-height:1.5">ErrorCode ierr</span> = giveMeFive(&a);</div><div>CHECK_ERROR_CODE(ierr)</div><div><span style="line-height:1.5">```</span></div><div>(In all fairness, the first example is missing a try-catch-block, but that only has to be in the outmost main routine; the CHECK has to be everywhere.)</div><div><br></div><div>The second example is three lines instead of one, `a` can in principle not be const, and you lose the nice and semantic way of returning a value from a method. There are more reasons in favor of exceptions, for example the fact that you can easily forget to check the return code (as is happening many times across MOAB). You cannot forget exceptions.<span style="line-height:1.5"> </span><span style="line-height:1.5">Return values are indeed standard in many languages (Fortran, C, etc.), but really only in those without the concept of exceptions.</span></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">Anyhow. I thought that this must be a controversial issue, and I don't want to start a big discussion about it. (Particularly given the fact that there has been one not too long ago.)</span></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">I'll do my best to help out adding the missing return value checks.</span></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">> </span><span style="line-height:1.5">If you inserted a missing error check and it resulted in a failing</span></div>> test, we definitely want to know the test case and possibly the<br>> reason.<div><br></div><div>Let's work our way through the easy catches [1], the tricky one will soon come to light.</div><div><br></div><div>Cheers,</div><div>Nico</div><div><br></div><div><br></div><div>[1] <a href="https://bitbucket.org/fathomteam/moab/pull-requests/176/coverity">https://bitbucket.org/fathomteam/moab/pull-requests/176/coverity</a></div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Nov 25, 2015 at 4:06 PM Vijay S. Mahadevan <<a href="mailto:vijay.m@gmail.com">vijay.m@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> I just sat down to fix a bunch of coverity warnings about using<br>
> uninitialized fields, unchecked return values etc.<br>
<br>
Yes, these should be fixed. I think cppcheck didn't catch these<br>
issues, so having coverity complaining about this is good.<br>
<br>
> It's not done by default, and if you actually check it, it clutters up the code<br>
<br>
This is the standard error checking model in C-based codes. Why do you<br>
call this clutter ? I am sure we can implement a macro to reduce the<br>
amount of typing necessary but I didn't see a big value add. We can do<br>
that now actually, to improve readability.<br>
<br>
> Also, function that naturally return void have to return `int`, and in fact<br>
> no function can return anything else _but_ `int` if it wants to do error<br>
> checking.<br>
<br>
Again, this is the error signature most commonly used in MPI, PETSc<br>
etc. We did some analysis on which error handler to use that would<br>
have minimal impact on the performance of the library. After several<br>
profiling studies and detailed discussions [1], we decided to go with<br>
the error handling macros with int-based return codes.<br>
<br>
If your function returns void, you can use MB_CHK_ERR_RET or<br>
MB_CHK_ERR_CONT macros [2]. You do not always need to use MB_CHK_ERR.<br>
This needs to be changed uniformly in the library.<br>
<br>
> Needless to say, this policy isn't followed strictly (in constructure, for<br>
> example, you even cannot return an int, thus cannot check for errors<br>
> MOAB-style).<br>
<br>
Yes, we need to invest more time in making the whole structure<br>
consistent. As I said before, returning an int is not mandatory. Take<br>
a look at the ErrroHandler class and the macros in there.<br>
<br>
> I'm sure you're all aware of it, but I would like you to know that this is<br>
> leading to actual errors: Inserting some MB_CHK_ERRs here and there lead me<br>
> to a failing test. I still need to dig out what exactly is the cause, but I<br>
> suspect a failing MB_CHK_ERR.<br>
<br>
If you inserted a missing error check and it resulted in a failing<br>
test, we definitely want to know the test case and possibly the<br>
reason. None of the tests should be that fragile and so I'm a bit<br>
surprised.<br>
<br>
Vijay<br>
<br>
[1] <a href="http://lists.mcs.anl.gov/pipermail/moab-dev/2014/006032.html" rel="noreferrer" target="_blank">http://lists.mcs.anl.gov/pipermail/moab-dev/2014/006032.html</a><br>
[2] <a href="http://ftp.mcs.anl.gov/pub/fathom/moab-docs/ErrorHandler_8hpp.html#a2075e361021042e2343145c70e8b5ea5" rel="noreferrer" target="_blank">http://ftp.mcs.anl.gov/pub/fathom/moab-docs/ErrorHandler_8hpp.html#a2075e361021042e2343145c70e8b5ea5</a><br>
<br>
On Tue, Nov 24, 2015 at 8:05 PM, Nico Schlömer <<a href="mailto:nico.schloemer@gmail.com" target="_blank">nico.schloemer@gmail.com</a>> wrote:<br>
> Hi everyone,<br>
><br>
> I just sat down to fix a bunch of coverity warnings about using<br>
> uninitialized fields, unchecked return values etc. I couldn't help but<br>
> notice that the return-value strategy for error interception is flawed. It's<br>
> not done by default, and if you actually check it, it clutters up the code<br>
> like<br>
> ```<br>
> ErrorCode rval<br>
> rval = do_something(foo, bar);<br>
> MB_CHK_ERR(rval);<br>
> ```<br>
> Also, function that naturally return void have to return `int`, and in fact<br>
> no function can return anything else _but_ `int` if it wants to do error<br>
> checking.<br>
><br>
> Needless to say, this policy isn't followed strictly (in constructure, for<br>
> example, you even cannot return an int, thus cannot check for errors<br>
> MOAB-style).<br>
><br>
> I'm sure you're all aware of it, but I would like you to know that this is<br>
> leading to actual errors: Inserting some MB_CHK_ERRs here and there lead me<br>
> to a failing test. I still need to dig out what exactly is the cause, but I<br>
> suspect a failing MB_CHK_ERR.<br>
><br>
> The fix for all this clearly are exceptions.<br>
><br>
> Cheers,<br>
> Nico<br>
</blockquote></div>