Remove unnecessary null pointer checks

[elfring]

An extra null pointer check is not needed in functions like the following.

• cineon::Codec
• cineon::Writer::WriteElement
• dpx::RunLengthEncoding::Reset

[lgritz]

Hi, although I 100% agree that the null check is unnecessary here, I'm hesitant to change it in these three particular places. Both the Cineon and DPX code came from external projects (with permission), and we try not to change the code within those particular files when not necessary, so that if we ever wanted to "re-pull" the original source (say, if bugs were fixed there), any unnecessary changes on our end would be lost if we replaced the files, or if we tried to combine them, might lead to more complicated diffs or possibly a tricky merge.

On the other hand, I don't think those files have ever been re-pulled from their original sources since they were first embedded in OIIO. So maybe I'm worrying for nothing and we should just consider it "our code" for all practical purposes. (On the third hand, if it were truly our original code, we probably would have not only removed the null checks, but removed the raw delete and made those variables unique_ptr. So perhaps we've implicitly made the decision to have slightly less nice code in those particular files, for the sake of minimizing divergence from the original upstream source?)

I'm on the fence here. Fixing these little spots don't complicate any future diffs or merge very much, but it doesn't feel like there's any concrete gain, either, so I'm not sure it's worth the acceptance of any future risk of merge conflicts.

Are these causing any problems? It kind of feels like it's just an automated scan of the code base.

[elfring]

It kind of feels like it's just an automated scan of the code base.

I performed a small source code analysis to find a few update candidates.

• How do you think about to delete unnecessary null pointer checks at other source code places then?
• Would you like to increase the usage of smart pointers instead?

[lgritz]

How do you think about to delete unnecessary null pointer checks at other source code places then?

I'm all for it! My hesitation is specific to the dpx and cineon modules because they were code that was imported from other projects. I'm just a little leery of "unnecessary" deltas making any future comparison or merge more difficult, in that one case. My hesitation isn't absolute, of course -- if there was a change that clearly improved the performance or correctness in measurable ways, I wouldn't be averse to diverging from the original sources.

Would you like to increase the usage of smart pointers instead?

Yes, and we have been. I'm not sure it's necessary to go out of our way looking for them in regions of the code that have never been problematic (unless somebody really wants to take this on), but certainly any time we are making modifications to a region code for other reasons, that's the kind of thing we should have an eye to cleaning up and improving while we're changing things. And certainly for new code, we strongly prefer smart pointers rather than raw new/delete.

[elfring]

… if there was a change that clearly improved the performance or correctness in measurable ways, …

• Would anybody dare to measure effects of questionable extra checks in the discussed source code?
• Could a corresponding source code reduction influence software characteristics in desirable ways?

…, that's the kind of thing we should have an eye to cleaning up and improving while we're changing things.

How do you think about to take another look at any remaining update candidate?

[lgritz]

Would anybody dare to measure effects of questionable extra checks in the discussed source code?

We measure all the time. But we don't need to measure in this case -- extra checks like this may be important in inner loops, but here we're talking about a couple pointer comparisons once per image file that we read, for two file types that aren't among the most common that we encounter. I swear saving those couple nanoseconds will have no measurable impact.

Could a corresponding source code reduction influence software characteristics in desirable ways?

Yes. We like simpler code. We like redundant checks. We like smart pointers and automatic memory management where applicable.

Like I said, any hesitation on my part is 100% an artifact of the changes being in these particular two source files, and I would jump at the chance to make exactly this fix anywhere else in the code.

How do you think about to take another look at any remaining update candidate?

Any remaining candidates, yes. The one you point to... I don't think that's the same, because glDeleteProgram() probably has an error if the thing you pass it is not a valid program, right?

---

Now, all that being said, I took another look at the changes you proposed in the cineon and dpx code, and I think I overreacted -- it's in simple parts of the code, I doubt those changes will create a problem for re-merges later. So if you want to propose the fixes as a PR so all I have to do is hit the 'merge' button, I'll accept them. (Whereas reporting them as an "issue" is saying "I want YOU to fix them", and the threshold for getting me to put my own time into that is much higher.)

[elfring]

…, any hesitation on my part … in these particular two source files, …

Do any other software developers (or maintainers) need to get informed about possible small adjustments in these modules?

Any remaining candidates, yes.

Are you aware that some OpenGL functions tolerate passed zero handles (similar to null pointers together with other resource release functions)?