Error handling fixes

[darbyjohnston]

As pointed out by @gfgit, there is an issue with the error handling in the new C++ functions Composition::child_at_time(), Composition::children_in_range(), Composition::children_if(), and SerializableCollection::children_if().

The code currently looks like this:

if (!error_status) {
...
}

But should look like this (similar to the surrounding code):

if (*error_status) {
...
}

We don't need to check whether error_status is null since the API assumes that it is a valid pointer.

[meshula]

Repeating the notes on the PR here. The bug being fixed isn't a bug...

The reason the null checks were on the error_status was because we pass in a pointer, that can be null, which causes crashes upon dereference. Clearly that's confusing as there's ambiguity as to whether the error status is supposed to be checked, or if the error status is merely a place to write.

This code is meant to write to error_status, if error_status was applied. Not, if error_status pointer was supplied return as if an error occurred.

I believe the real issue, to @gfgit's original review, is that we are using a C style idiom in a C++ interface. Having a pointer to a place to store any errors in the interface is the root source of the issue. I'm open to a new interface proposal.

[darbyjohnston]

I think there are two issues here; first, the PR does fix a legit bug in the error handling of the new functions I added. The changes in the PR will fix the functions so that they properly catch errors and return from the function when an error occurs.

The second issue is about passing error_status as a pointer in the C++ API; if it's expected to always be a valid pointer that should probably be documented. Or maybe change the API so that error_status is passed as a reference so there's no ambiguity.

[meshula]

I'm not comfortable with the notion that !error_status was interpreted by readers as evaluating a status, nor am I comfortable with *error_status evaluating status. I'd prefer an optional as the only variant that actually expresses the intent that there may be no ErrorStatus. Also I'd prefer that asterisk is not the test, but an explicit test, something like

void someFunc(..., optional<ErrorStatus> es) {
    if (es && es->error_occurred())
        return;
}

Anything else, and we're asking people to memorize ambiguous semantics.

I'm not fond of ErrorStatus& unless the pointer is changed to a reference across the entire API. But then, we have a C++ ism that doesn't match to our wrapping languages except in an unsafe way. In other words, in C, we have to pass by reference, and we are back to square one of broken semantics. Swift, Python, and Java only magnify the mismatch.

[darbyjohnston]

The asterisk isn't even the actual test, it dereferences the pointer so that ErrorStatus::operator bool() can be used to test whether the outcome is != OK. I think that combination of pointers and operators is maybe a bit confusing.

Using an optional is interesting, in that case would it be defaulted to null for all of the functions? The downside is that changing the API would require all C++ code that currently uses OTIO to be updated. Maybe not an unreasonable thing to do before v1.0. :)

[meshula]

Yes, it is the bool() operator that I am explicitly objecting to. Sadly, on further thought, passing an optional has the same problem I mentioned versus language bindings. For now, I'd be satisfied with a null pointer test followed by a test that is not a bool() conversion.

[darbyjohnston]

If we are allowing error_status to be null, how about defaulting it to null for all functions? That way it's self-documenting that it can be null, and allows a user of the API to ignore it if they don't want to do error checking.

[ssteinbach]

Should we have a macro for checking this? something that wraps up the null check and returns a default error status thing if it is a nullptr?

[ssteinbach]

Thinking about it some more, if we used a std::pair to return {error, value} instead of error_status* + mutable reference as an argument, that might work better

[darbyjohnston]

Maybe instead of a macro, a static function on ErrorStatus:

static bool is_ok(ErrorStatus* es)
{
    return es && Outcome::OK == es->outcome;
}

Used like:

if (!ErrorStatus::is_ok(es))
{
    // Handle error...
}

That's an interesting idea about std::pair; so the function:

TimeRange range_of_child_at_index(int index, ErrorStatus* error_status) const;

Would look like this instead?

std::pair<TimeRange, ErrorStatus> range_of_child_at_index(int index) const;

That might get a bit complex with some of the other functions:

std::pair<std::pair<optional<RationalTime>, optional<RationalTime>>, ErrorStatus>
    handles_of_child(Composable const* child) const;

[ssteinbach]

Right. I would probably put the error first in the pair. And you could wrap that in a macro:

#define ERROR_WRAPPER(type) std::pair<ErrorStatus, type> so you would have: ERROR_WRAPPER(std::pair<optional<RationalTime>, optional<RationalTime>>) handles_of_child(Composable const* child) const;

maybe? just a thought. @meshula, any further comments? We could discuss it at the TSC meeting if people are interested in a change like this. I'm not sure if there are other C++ libraries out there with similar patterns that we could look at, but this came out of a conversation @meshula and I were having in the context of the zig programming language.

[meshula]

Another variant is to not use a pair, but an explicit template because we don't actually want the polymorphic nature of pair. e.g.

template<typename T> struct CheckedValue {
    explicit CheckedValue(ErrorStatus es) : status(es) { if (es == ErrorStatus::NoProblemo) throw Whatever; } }
    explicit CheckedValue(const T& val) : status(ErrorStatus::NoProblemo), value(val) {}
    explicit CheckedValue(T&& val) :  status(ErrorStatus::NoProblemo), value(std::move(val)) {}
    const ErrorStatus status;
    const T& checked_value() const { if (status != ErrorStatus::NoProblemo) throw Whatever; return value; }
private:
    const T value;
};

[darbyjohnston]

I wonder if that would make things more difficult for the language wrappings? It would also require updating all of the bindings and C++ client code which is a fair bit of work.

After thinking about it some more, my preference would be keeping the API mostly the same but with a couple changes to help improve the clarity:

• Add nullptr defaults to the C++ function declarations that take error_status *. That would document that error_status can be a nullptr, and also allow users to avoid error handling if they don't need it.
• Replace ErrorStatus::operator bool() with a static member function that checks if error_status is a valid pointer, and if it is set to OK. This will allow error_status to be null and hopefully make the code a bit more clear.

I think those two changes would help a lot and also be relatively easy to make. It also shouldn't affect the bindings or C++ client code.

[meshula]

I don't expect it to be difficult to bind per se. The advantage is that modern languages generally have multiple returns, so the class would map onto a multiple return. It would be a tedious and time consuming change however!

Your latter suggestion is the lightest touch, and gets rid of the operator bool, so I'd personally be fine with that.

[darbyjohnston]

I updated the PR with my proposed changes, and also converted it to a draft since we are still iterating on the issue.

I didn't remove ErrorStatus::operator bool yet, since that would require updating all of the code and I just wanted to get feedback about the proposed changes. I just updated the code in the original PR to use the new member function instead of the operator.