add is_error and fix error handling #1061
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1061 +/- ##
==========================================
- Coverage 86.32% 86.19% -0.14%
==========================================
Files 191 191
Lines 19004 19041 +37
Branches 2105 2105
==========================================
+ Hits 16406 16412 +6
- Misses 2052 2083 +31
Partials 546 546
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Given that the overwhelming number of if() statements are !...is_ok(error_status), would it make more sense to provide ErrorStatus::is_error?
if (ErrorStatus::is_error(error_status)) {...}
(this would just be to test positives rather than negatives).
We could additionally promote this to a top level function or macro:
IS_ERROR(error_status)
is_error(error_status)
Rather than one scope on the type.
Apologies for the slow/late response!
There was a problem hiding this comment.
This looks great! one REALLY small change to a comment and I think we can land this! Thanks @darbyjohnston!
Change comment to include *
|
Looks great, thanks @darbyjohnston! |
Fixes #1060
These changes fix the error handling in
Composition::child_at_time(),Composition::children_in_range(),Composition::children_if(), andSerializableCollection::children_if()to properly check the error_status and return from the function.I also added an additional Python test for the various options to
children_if()/each_child(), though it might not be necessary if it doesn't increase test coverage. I was hoping to test the various error conditions that were fixed, though it's not obvious how to trigger them since error_status is passed down to so many different functions.