Re: patch nag: handle CORBA errors

[Christian Neumair <chris gnome-de org>]

Am Donnerstag, den 14.09.2006, 21:53 +0200 schrieb Alexander Larsson:
> 
> Is that really right? "!found || error != NULL" is the same as
> "!(found
> && error == NULL)", so we assert if we find something and get no
> error?

Yes, that is right.

> What is it meant to do? The original looked more right to me, either
> we
> found something, or we have an error.

This code is used to ensure that this monitor was not registered before
for this metafile, so basically we assert
  !found .

The
  error != NULL
statement ensures that we don't evaluate the value of "found" if an
error occured. Swapping those two might have been more clear, or maybe
we could have used a

if (error != NULL) {
      ...
      return;
} else {
      g_assert (!found);
}

block to prettify it, but I'm not a big fan of if ... else statements
when both branches will end up in totally different parts of the program
after execution (i.e. return makes us jump). Moving the assert down
after a single "if (error != NULL)" branch might have made it look as if
it wasn't really coupled to the original function call.
the

Of course, we could also have replaced the entire rest of the function
with

if (error == NULL) {
        g_assert (!found);
        /* register monitor */
} else {
        /* handle error */
}

If you think it's worth switching over to that, I'm also all for it, but
in the past some of my whitespace and mess-with-cvs-history patches were
rejected, so I was/am a bit reluctant.

-- 
Christian Neumair <chris gnome-de org>