Skip to content

Review: ImageBuf error reporting beef-up#407

Merged
lgritz merged 4 commits intoAcademySoftwareFoundation:masterfrom
lgritz:lg-errors
Jul 23, 2012
Merged

Review: ImageBuf error reporting beef-up#407
lgritz merged 4 commits intoAcademySoftwareFoundation:masterfrom
lgritz:lg-errors

Conversation

@lgritz
Copy link
Collaborator

@lgritz lgritz commented Jul 17, 2012

Per review comments from Jeremy, here I've added a public error() function to ImageBuf so that apps can add errors to ImageBuf objects.

Hereby we adopt the convention that ImageBufAlgo functions bubble up error messages by attaching them to their "Result" IB (in addition to returning 'false').

Prototyped this idiom for IBA::render_text and "oiiotool --text". As an example:

$ oiiotool --create 640x480 3 -text:font=lg "Hi" -o out.tif
ERROR: -text:font=lg (Could not set font face to "lg")

If you guys like this, I will amend the pull request with such error reporting for the rest of the IBA/oiiotool functions.

@StefanStavrev
Copy link
Contributor

Why not return int instead of bool?

0 could mean success, and any other number will represent
an error ID. This way we would not clutter ImageBuf. Also
the ID can be index to vector of error messages specific for
that operation. And it is nice to have a mapping from integers
to error messages so you can refer to them in the documentation,
for example error 4 or whatever.

@lgritz
Copy link
Collaborator Author

lgritz commented Jul 17, 2012

If we were starting from scratch, I'm sure your argument would have merit (though I tend to strongly prefer a simple ok/fail return and then a way of querying a detailed error message, over the "error number / table of error names" approach).

But for better or worse, OIIO has throughout it a very consistent precedent of how return values and error queries work. Though reasonable people can disagree about its theoretical merits versus integer return codes, there is no doubt that using either one consistently is superior to having some parts of the API use one and other parts use the other. And we can't really change the parts that are already done to the other technique, there is just too much client app code that expects 'if (! oiiofunction()) { ...handle error...}" to work as it always did.

@lgritz
Copy link
Collaborator Author

lgritz commented Jul 17, 2012

I had some free time while waiting for benchmarks, so I quickly hacked this idiom into as many IBA functions as I could find, as well as their wrappers in oiiotool. We should have fairly helpful error reporting in oiiotool now.

@lgritz
Copy link
Collaborator Author

lgritz commented Jul 17, 2012

Excuse my having semi-accidentally included in this review a typo fix for my recent change to searchpath_find. Probably should have been separate, but it was a one-liner and I found it while making sure tests passed for this change.

lgritz added 4 commits July 19, 2012 16:15
* Add public error(...) so app code can associate an error with an ImageBuf.
* Thread safety for IB error handling.
…lt ImageBuf.

Implement this for IBA::render_text and collect it properly via oiiotool.
@lgritz
Copy link
Collaborator Author

lgritz commented Jul 23, 2012

Everybody ok with this?

@StefanStavrev
Copy link
Contributor

What about functions that take just one const ImageBuf &?

ImageBufAlgo::histogram is such example.

Is it ok to make it ImageBuf & instead, so that we can attach
errors to it?

Not a big deal maybe, but it will mess up the code style,
by passing read-only images as non-const.

@lgritz
Copy link
Collaborator Author

lgritz commented Jul 23, 2012

I believe that the way I wrote the error() function, it's declared as 'const', so you can add errors to a 'const ImageBuf &'.

lgritz added a commit that referenced this pull request Jul 23, 2012
ImageBuf error reporting beef-up
@lgritz lgritz merged commit ba933ad into AcademySoftwareFoundation:master Jul 23, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants