[MOAB-dev] rval errors

[Vijay S. Mahadevan]

> 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