[Cogl] [PATCH] Adds CoglError api

[Robert Bragg]

On Mon, Sep 3, 2012 at 2:08 PM, Neil Roberts <neil at linux.intel.com> wrote:
> Robert Bragg <robert at sixbynine.org> writes:
>
>> This patch also enforces a consistent policy for when NULL is passed as
>> an error argument and an error is thrown. In this case we log the error
>> and abort the application, instead of silently ignoring it.
>
> I'm not sure whether this is a good idea. In GLib, if you pass NULL for
> the GError argument it doesn't mean that you're not handling the error
> but it instead means you don't care what the details of the error are.
> It's the normal convention that functions that can throw a GError also
> have some return value to indicate whether there was an error (such as
> returning FALSE).
>
> For example if you call cogl_pipeline_set_depth_state and you have a
> fallback path for when the depth state isn't supported you can just
> check the return value without looking into the GError. If you are doing
> this every paint then it's quite nice to be able to avoid allocating the
> GError just to redundantly free it again.

It doesn't seem ideal to be repeatedly hitting an error in _set_depth,
every frame and if was a performance problem there would be several
approaches to solve that before needing to optimize Cogl's error
reporting, including setting up a template pipeline once with the
depth state or maintaining some application state to know not to
repeatedly set unsupported depth-state.

>
> I think this behaviour is potentially more useful than aborting for most
> of the errors that Cogl will throw and it's nice not to diverge from the
> GError semantics unless there is a good reason.

I guess I tend to find myself wanting the abort on error semantics
more often than not but I'm sure you're right that there are times
when that's not the most convenient approach. My hunch a.t.m is that
the aborting behaviour is more useful but it's hard to know for sure I
suppose.

I tend to think that if I really want to handle an error then an extra
step of passing an error pointer and freeing it as necessary is fine,
but if I don't care to handle the error and haven't implemented some
way to handle the error then I would at least want the app to abort at
that point because otherwise it's likely the app is no longer in a
consistent state.

Programmers being lazy I tend to think it's common for NULL to be
passed as an error pointer not because they aren't interested in the
specific details of the error as in your example, but literally
because we are often lazy and don't always implement code to handle
all errors. In this case if you do hit an error but don't have code
that deal with that then I think it would be best to cleanly abort
with a meaningful error at the point of error.

When I think of exceptions in languages such as C++, Java or
JavaScript etc, they follow a pattern of automatically aborting the
application if the developer didn't explicitly catch and handle an
exception. It feels to me that passing a valid error pointer is the
closest equivalent we have to wrapping code in a try{} block and so
this policy seems intuitively consistent with traditional exceptions
to me.

Something else I like about the abort on error approach is that we can
apply that consistently in a maintainable way, whereas we currently
have different functions applying different conventions. For example
the blend string apis use g_warning() if a NULL error is given to
print the error to the console. This was done on Owen's request
because he felt it's common that there's often no meaningful way an
app is going to handle blend string errors so we might as well save
them the effort of passing an error pointer to catch syntax errors. I
think that tends to agree with the 'programmers are lazy' thinking I
currently have. This basically goes a step further though and actually
aborts the program too which I think makes sense if we believe the
application isn't going to handle the error explicitly.

>
> Perhaps if we wanted to make the abort-on-error use case convenient then
> we could add another dummy value to pass instead of NULL. For example,
> _cogl_set_error and _cogl_propagate_error could detect when they are
> passed COGL_ABORT_ON_ERROR. This could be defined to (void*)0x1 or
> something.

I still tend to think that the most common reason for a NULL error
pointer is that a lazy programmer hasn't implemented code to handle
the error as opposed to them not being interested in the error details
so I'm not sure it would be good to require a particular enum be
passed to cause an abort because lazy programmers won't pass the enum
either.

Similarly though a COGL_IGNORE_ERROR could be defined to optimize the
_set_depth_state example you gave. I'm not sure that should really be
necessary unless we find a real example where the performance of
allocating CoglErrors shows up as a problem, but it's also a
possibility.

kind regards,
- Robert

>
> Regards,
> - Neil