TypesettingTools / SubInspector

Low-level subtitle inspection library.
MIT License
5 stars 3 forks source link

Error reporting #8

Closed Youka closed 10 years ago

Youka commented 10 years ago

assi_checkLine has a very dirty way of returning the check result, it would be better the function returns it directly and doesn't pollute the times input, additionally there's no way to return ASS errors so far.

A status function with error explanation (like a string) would be a better solution instead of letting every function just say "something was wrong".

torque commented 10 years ago

I don't consider it to be dirty. Programming by side effects seems to be a very common paradigm in C. It's up to the user to allocate the result array and it's up to them to free it. Because it's passed as a pointer, the values are modified in the function. This seems very straightforward to me.

Passing result as a modifiable reference also allows the return value to be a status code. I think it would be very inconsistent if assi_checkLine were to return a uint8_t* when every other api call returns an integer status code.

As for making error reporting useful, my thoughts were along these lines:

char * assi_getLastErrorMessage( ASSI_State *state ) {
    return state->lastError;
}

Overall, a user would do something like:

ASSI_State *state = assi_init( width, height );
if ( !state ) {
    puts( assi_getLastErrorMessage( state ) );
    exit( 9001 );
}
if ( assi_initEvents( state, 5000 ) ) {
    puts( assi_getLastErrorMessage( state ) );
    exit( 9001 );
}
// and so on
Youka commented 10 years ago

I don't mean checkLine should return uint8t**, i mean *_result[timesLength]. Getting single time checks could be an optional parameter but the summary should be returned by the function.

Functions should return their results, error handling can be done sidewise. They are already safe against wrong usage (or soon) and people should know how to use them correctly or wondering why nothing happened, so more focus on usability can be taken.

torque commented 10 years ago

Regardless of whether the return is a pointer to an array or a solitary value, I'm still inclined to disagree.

every other function in the api returns a status code. Now, I'll admit that none of the other ones are actually returning data back to the user, but I still think there should be consistency to api. Since this function can still error in libass or for other reasons, it should first and foremost be easy to check if it succeeded.

People knowing how to use them correctly is accomplished through proper documentation.

If checkLine were modified to only calculate if a line is completely invisible or not:

int dirty;
if ( assi_checkLine( state, eventIndex, &dirty ) ) {
    // handle error
}
if ( dirty ) {
    // handle dirty line
}

The problem as I see it with changing it so it returns a value is that you lose consistency, and you still need to do error checking before you can use the resulting value.

int dirty = assi_checkLine( state, eventIndex );
if ( /*some different way of checking for errors*/ ) {
    // handle error
}
if ( dirty ) {
    // handle dirty line
}