Closed jcupitt closed 1 year ago
General approach SGTM. I'd suggest not falling back to logging a message if the error pointer is NULL
, since that could encourage callers to generate log messages that their callers won't be able to intercept.
Hi Benjamin, thanks for having a look.
My idea was to make it possible to flag errors in code paths that don't have a DcmError**
parameter (there are a few of these inside libdicom). Maybe it'd be better to force us to add the extra parameters instead.
How about logging these messages as DEBUG?
I think it makes sense to do both together. DEBUG logging would be useful for debugging missing error checks, but we should also be sure to add DcmError **
parameters to any function that might need one.
Yes, exactly. I changed it to log NULL errors to DEBUG, changed the docs to say you should always pass an error object, and changed dcm-dump
to use DcmError
.
Sorry to ping you, but could someone approve this? I can't merge otherwise :(
Thanks @hackermd, merged! I have two more queued up :(
This (huge!) PR adds GError-style error handling to libdicom, plus docs, plus tests.
Resolves https://github.com/ImagingDataCommons/libdicom/issues/17
Here's the proposed new style:
Summary:
DcmError **
pointer as their first argumentDcmError *
where the error return should be writtendcm_error_clear()
to wipe the pointer againIn the library, code that raises errors looks like this:
Summary:
dcm_error_set()
and return immediatelyerror
is either NULL, or a pointer to a NULL on function entrydcm_error_set()
cannot be called again without clearing the error pointererror
is NULL, the error will be logged rather than returned in the pointerI also fixed a few small bugs and leaks I found as I went.
I removed optimisation from the default debug build since it was making debugging tricky. I also removed asserts from production builds -- this is more in line with practice in other libraries (glib, for example, will not
abort()
if you get a parameter wrong).In my opinion, detailed review is not very useful at this stage, since libdicom is likely to be changing pretty quickly over the next couple of weeks. If the outline above sounds reasonable, I think we should just merge it.
Virtual IO next!