[MOAB-dev] rval errors

[Nico Schlömer]

> This is the standard error checking model in C-based codes. Why do you call
this clutter ?

Because of
```
const int a = giveMeFive();
```
vs.
```
int a;
ErrorCode ierr = giveMeFive(&a);
CHECK_ERROR_CODE(ierr)
```
(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.)

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. Return values are
indeed standard in many languages (Fortran, C, etc.), but really only in
those without the concept of exceptions.

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.)

I'll do my best to help out adding the missing return value checks.

> If you inserted a missing error check and it resulted in a failing
> test, we definitely want to know the test case and possibly the
> reason.

Let's work our way through the easy catches [1], the tricky one will soon
come to light.

Cheers,
Nico


[1] https://bitbucket.org/fathomteam/moab/pull-requests/176/coverity

On Wed, Nov 25, 2015 at 4:06 PM Vijay S. Mahadevan <vijay.m at gmail.com>
wrote:

> > I just sat down to fix a bunch of coverity warnings about using
> > uninitialized fields, unchecked return values etc.
>
> Yes, these should be fixed. I think cppcheck didn't catch these
> issues, so having coverity complaining about this is good.
>
> > It's not done by default, and if you actually check it, it clutters up
> the code
>
> This is the standard error checking model in C-based codes. Why do you
> call this clutter ? I am sure we can implement a macro to reduce the
> amount of typing necessary but I didn't see a big value add. We can do
> that now actually, to improve readability.
>
> > Also, function that naturally return void have to return `int`, and in
> fact
> > no function can return anything else _but_ `int` if it wants to do error
> > checking.
>
> Again, this is the error signature most commonly used in MPI, PETSc
> etc. We did some analysis on which error handler to use that would
> have minimal impact on the performance of the library. After several
> profiling studies and detailed discussions [1], we decided to go with
> the error handling macros with int-based return codes.
>
> If your function returns void, you can use MB_CHK_ERR_RET or
> MB_CHK_ERR_CONT macros [2]. You do not always need to use MB_CHK_ERR.
> This needs to be changed uniformly in the library.
>
> > Needless to say, this policy isn't followed strictly (in constructure,
> for
> > example, you even cannot return an int, thus cannot check for errors
> > MOAB-style).
>
> Yes, we need to invest more time in making the whole structure
> consistent. As I said before, returning an int is not mandatory. Take
> a look at the ErrroHandler class and the macros in there.
>
> > I'm sure you're all aware of it, but I would like you to know that this
> is
> > leading to actual errors: Inserting some MB_CHK_ERRs here and there lead
> me
> > to a failing test. I still need to dig out what exactly is the cause,
> but I
> > suspect a failing MB_CHK_ERR.
>
> If you inserted a missing error check and it resulted in a failing
> test, we definitely want to know the test case and possibly the
> reason. None of the tests should be that fragile and so I'm a bit
> surprised.
>
> Vijay
>
> [1] http://lists.mcs.anl.gov/pipermail/moab-dev/2014/006032.html
> [2]
> http://ftp.mcs.anl.gov/pub/fathom/moab-docs/ErrorHandler_8hpp.html#a2075e361021042e2343145c70e8b5ea5
>
> On Tue, Nov 24, 2015 at 8:05 PM, Nico Schlömer <nico.schloemer at gmail.com>
> wrote:
> > Hi everyone,
> >
> > I just sat down to fix a bunch of coverity warnings about using
> > uninitialized fields, unchecked return values etc. I couldn't help but
> > notice that the return-value strategy for error interception is flawed.
> It's
> > not done by default, and if you actually check it, it clutters up the
> code
> > like
> > ```
> > ErrorCode rval
> > rval = do_something(foo, bar);
> > MB_CHK_ERR(rval);
> > ```
> > Also, function that naturally return void have to return `int`, and in
> fact
> > no function can return anything else _but_ `int` if it wants to do error
> > checking.
> >
> > Needless to say, this policy isn't followed strictly (in constructure,
> for
> > example, you even cannot return an int, thus cannot check for errors
> > MOAB-style).
> >
> > I'm sure you're all aware of it, but I would like you to know that this
> is
> > leading to actual errors: Inserting some MB_CHK_ERRs here and there lead
> me
> > to a failing test. I still need to dig out what exactly is the cause,
> but I
> > suspect a failing MB_CHK_ERR.
> >
> > The fix for all this clearly are exceptions.
> >
> > Cheers,
> > Nico
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mcs.anl.gov/pipermail/moab-dev/attachments/20151125/ebd5664e/attachment.html>