api(fmath.h): apply OIIO_NODISCARD (integer helpers, float utilities)

[luna-y-kim]

Description

Part of #4781
No callers ignore the return value.

Tests

N/A

Checklist:

• I have read the guidelines on contributions and code review procedures.
• I have read the Policy on AI Coding Assistants
  and if I used AI coding assistants, I have an Assisted-by: TOOL / MODEL
  line in the pull request description above.
• (N/A) I have updated the documentation if my PR adds features or changes
  behavior.
• (N/A) I am sure that this PR's changes are tested in the testsuite.
• I have run and passed the testsuite in CI before submitting the
  PR, by pushing the changes to my fork and seeing that the automated CI
  passed there. (Exceptions: If most tests pass and you can't figure out why
  the remaining ones fail, it's ok to submit the PR and ask for help. Or if
  any failures seem entirely unrelated to your change; sometimes things break
  on the GitHub runners.)
• My code follows the prevailing code style of this project and I
  fixed any problems reported by the clang-format CI test.
• (N/A) If I added or modified a public C++ API call, I have also amended the
  corresponding Python bindings. If altering ImageBufAlgo functions, I also
  exposed the new functionality as oiiotool options.

[luna-y-kim]

The job "Linux ARM latest releases" failed with the log "The operation was canceled." It passed on my fork. I'm not sure what's causing it.

[luna-y-kim]

I tried to re-trigger CI by force-push, and now two Linux ARM jobs failed with exit code 143.

[lgritz]

Because these died in the "dependencies" stage, they are clearly unrelated to your PR. Probably means that some site we must download from, like a package repository, may be temporarily down. Usually just waiting it out a few hours is all it takes. And you shouldn't need to re-push; there is a "re-run jobs" button on the status page for the CI run, and if you press it, it gives you the option of rerunning just the failed jobs.

[lgritz]

I'm sorry for not noticing this before and explaining it in the original ticket, but it's only now that I'm seeing a few more of these OIIO_NODISCARD related pull requests come in that I want to revise the guidance about what to do. It pains me to question the value of work only after people have done it, but in this case I want to redirect your team's efforts toward high value tasks in the future.

I think that there are functions that are not worth doing this for, because discarding the return value is not a mistake that anybody is likely to ever actually make. I think the set of functions you annotated in this PR are likely in this category -- and the empirical evidence is that after annotating them, you found no place in our code where we inadvertently discarded those values, in contrast to #5196 that you did earlier, or #5201 is an even better example, that uncovered some error values we ignored elsewhere in our own code.

As I think more about it, I believe the priority order for these annotations are as follows (in order of highest to lowest value):

1. Functions that return error status (needing OIIO_NODISCARD_ERROR), because finding the places where the errors are ignored will tangibly improve safety, robustness, and correctness. It is probably common to ignore these return values, it makes programs incorrect in subtle ways (it works fine until you encounter an error condition in the wild), and so there is a high value in fixing that.

   An example of this is ImageInput::read_scanlines() -- it returns an error status, robust code really should be checking it because errors could occur and need to be handled by the calling code, and it's common for people to not bother or not realize that there is a return value.

2. Functions that return real values (not errors) that are expensive to construct, or for which it might be easy for users to not realize that there is a return value at all. These might be bugs or correctness issues in user code, though probably they are obvious bugs that will be quickly detected (e.g. if you don't realize the result is the return value and think it's modifying one of the reference parameters instead, you will probably detect fairly quickly that you've done something wrong).

   An example of this is ImageBufAlgo, whose functions tend to come in pairs: one that takes a destination image by reference as a parameter and returns a bool error status, and a twin that returns the result image. The first should be annotated with OIIO_NODISCARD_ERROR, and the second should get OIIO_NODISCARD, because the functions look similar and it might not be clear that it is returning the actual result, and it's a VERY expensive result to compute, so ignoring it is not only wrong, but also unnecessarily expensive.

3. Functions that are "pure functions" -- they have no side effects and no reference parameters that might be mutable values that are changed, so the only point in calling them is to get the result. It's not really a correctness issue, since they have no side effects. It might be a performance issue, if it's an expensive thing to compute, but on the other hand, the optimizer may catch it and eliminate the call in practice. But mostly, I suspect that this is not a mistake that people will make in real life.

---

So I think that for now, we should not bother with item 3. At the very least, let's fully finish 1 and 2 first, and then for 3-type items, let's propose it before doing the work (e.g., "I'm thinking of annotating all the 'fast math` functions, do you think it's worth it?").

[luna-y-kim]

And you shouldn't need to re-push; there is a "re-run jobs" button on the status page for the CI run, and if you press it, it gives you the option of rerunning just the failed jobs.

I don't have permission to re-run the failed jobs on the OIIO repo, and that is why I tried to re-push to re-trigger the whole CI.

[luna-y-kim]

(Small note: I'm an individual contributor rather than part of a group.)

The revised priorities make sense to me. One thing I'd like to clarify is:

which it might be easy for users to not realize that there is a return value at all

could the function floorfrac be an example of that? It writes *xint and also returns the fraction.

/// Return (x-floor(x)) and put (int)floor(x) in *xint.  This is similar
/// to the built-in modf, but returns a true int, always rounds down
/// (compared to modf which rounds toward 0), and always returns
/// frac >= 0 (compared to modf which can return <0 if x<0).
inline OIIO_HOSTDEVICE float
floorfrac (float x, int *xint)
{
    float f = std::floor(x);
    *xint = int(f);
    return x - f; 
}

[lgritz]

could the function floorfrac be an example of that? It writes *xint and also returns the fraction.

There's not usually a "right" answer, it's ultimately a judgment call about whether we think the people likely to know about and use the call are likely to ignore a return value that is important to check.

I think a pure function like fast_sin() will never be misused in this way.

floorfrac is a little more ambiguous, but I think it's reasonable to include the annotation here.

[luna-y-kim]

Thank you for the explanation. I've been skimming fmath.h, and so far I haven't found other functions worth annotating. I'm leaning toward closing this PR and moving on to other files. Although, if you prefer, I'm happy to keep the annotation for floorfrac, whichever you think is best!

[lgritz]

We discussed this in the TSC meeting today, and @ThiagoIze argues that we SHOULD annotate these "pure functions" in my tier 3. I was claiming that since the only point of the function is to use the return value, nobody would make the mistake of not using it. But his position is the flip side: because the return value is the only point, it's definitely a bug if it is not used, and he has seen this bug happen in the wild (he does admit that it's a fairly rare bug, but it is a real bug).

So, ok, I'm not inclined to reject such annotations, though I still do believe that it's a higher priority to attack the tier 1 and 2 annotations, just in terms of having the work put into annotations bring the most good to users.

Thiago also points out that not all status/error returns are created equal: There are some that are of no consequence to really checking (or the error is vanishingly rare), so is it really worth the clutter of always checking? (It's not immediately clear if we have any functions like this in OIIO.) But there are other cases where the error really could happen, and where failing to check could lead to a cascade of other failures -- like the example of ImageInput::read_scanlines(), which is a kind of failure that really could happen, and if you don't notice it, whatever you do next involving those pixels you tried to read will be wrong, because the pixels won't really have the value you thought came from the file, so that's a case where it's extremely important to force people to check.

I think this just goes to show that while adding the annotation to a function is itself a trivial act, there can be some subtlety in judging exactly which functions should be so annotated.

[lgritz]

LGTM. I am convinced that these do not hurt anything and just might help somebody catch a bug someday.