feat(ImageBuf): auto print uncaught IB errors

[lgritz]

Advanced users know to check error returns and retrieve errors.

New users often just try OIIO calls with no error checks when they don't get behavior they want, ask questions to the dev team. We hypothesize that if they only saw the text of the error messages that they never retrieved, they could probably self-diagnose many of their problems.

This patch makes an ImageBuf, when it is destryed, print any pending error messages that were never retrieved. It happens by default, but can be disabled by setting the new global attribute "imagebuf:print_uncaught_errors" to 0 (it defaults to 1).

This doesn't cover everything, but for ImageBuf and ImageBufAlgo, maybe it will help some users? It could annoy advanced users who intend to not retrieve error messages (why?) but still don't want this printed. But that's who the new attribute is for.

[lgritz]

Does anybody have opinions on this? There's a bit of a tradeoff between possibly printing error messages that are unwelcome, versus making people who are not properly checking for errors more aware of errors they are encountering without even realizing. There is not a right answer here, it's more of a choice between which will help or annoy more people.

[nrusch]

My general sentiment is that, if you're competent enough to be trying to write code that uses a semi-complex API like OIIO, you have a responsibility to do your best to understand that API (including its error conditions). Basically, RTFM.

However, I also understand that some people just aren't willing to put in the effort, and that there's a practical motivation on the maintenance side to try and reduce the number of "help, my code isn't working"-style Github issues.

I still personally think this feels a little niche for default behavior (e.g it doesn't help if the user's code is crashing, it doesn't help users of other APIs, etc.). Thus, I would lean toward disabling it by default, but maybe a compromise would be to add a build option to control the default state, while still allowing it to be explicitly enabled/disabled at runtime via a global option.

[lgritz]

I can certainly add a build-time option to control the default, though many users get their OIIO from a pre-built distribution, so they're still in the boat of needing to explicitly turn it on or off to get the behavior they desire. And the default has to be to enable this behavior, since the same people who don't know to check for error returns (i.e. the people you want to be using this feature enabled) aren't going to know all the esoteric build-time control to turn it on, either.

Let me kind of rebut a couple points so you know where I'm coming from on this:

I don't necessarily think it's a question of people not RTFMing or putting in the effort. The way I approached it is, let's assume it's a smart developer who wants to be thorough, but there is a transition period while they are coming up to speed on the API, maybe they are trying the first few things out and haven't elaborated their test programs with error handling on every last call yet. They are seeing unexpected behavior and just don't know why. So they report it to us as either a bug, or even just a request for help. Here is an example in a thread we both participated in, but this kind of thing happens frequently.

The "RTFM" part could just as well mean "if you don't want this behavior in your app, you should know to set the option that turns it off."

Remember that this is not printing every error as it happens. It's only printing when you destroy an IB that encountered errors but nobody ever called geterror() to retrieve them. We are presuming that somebody is going to want to know that this error happened and we shouldn't just sent it down a black hole.

So my assumption here is that experienced users will either (a) be checking all the error results diligently, so there will be no unretrieved errors for this code to print, or (b) will explicitly disable this behavior with the option. But for inexperienced users, this could be a big help as they are coming up to speed with the library.

[nrusch]

And the default has to be to enable this behavior, since the same people who don't know to check for error returns (i.e. the people you want to be using this feature enabled) aren't going to know all the esoteric build-time control to turn it on, either.

Yes, I agree that the default should be to build OIIO with imagebuf:print_uncaught_errors set to 1 by default, and then the build option could be used to change the default to 0.

Remember that this is not printing every error as it happens. It's only printing when you destroy an IB that encountered errors but nobody ever called geterror() to retrieve them.

Thanks for reiterating this, as I had elided that particular nuance while thinking about it in the context of our OIIO usage. The fact that it's only "unexamined" errors makes it much more palatable. I think you've mostly changed my mind.

[lgritz]

I'm trying to think: I'm at a fairly sophisticated place. Would I want our build to set the default to 0? No, I don't think so. Even at my studio, I want somebody to be forced to see and be annoyed by any uncaught errors.

[lgritz]

Does anybody have any remaining reservations about this fix? Keeping in mind that:

• It doesn't just spew errors as it goes, it only prints errors an ImageBuf encountered that were never retrieved with ib.geterror(), at the time the ImageBuf is destroyed.
• An app can turn it off with OIIO::attribute("imagebuf:print_uncaught_errors", 0) if for some reason it wants to neglect to check and retrieve errors and yet still not have it print the errors that occur.
• If it turns out we don't like this behavior in practice, we can change it later. It's just an attribute, not a public API change that breaks compatibility, and it doesn't change any current non-error-inducing behavior.

Absent further objections, I will merge it this week.