added implementation of DS_ErrorCodeToErrorMessage

[imskr]

Implementation of an API to get textual descriptions of error codes.

Fixes #2773

[community-tc-integration]

No Taskcluster jobs started for this pull request

The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

[imskr]

@reuben I have added the function. Now, what should I add further to it?

[reuben]

Thanks for the PR! I have a few suggestions for better phrasing, and the function needs to be documented in deepspeech.h.

[kdavis-mozilla]

@reuben @imskr Note that the way these strings are created, in static memory and not the heap, contradicts with the documentation of DS_FreeString() which says that it is used to "Free a char* string returned by the DeepSpeech API".

People will likely not catch this subtle difference and try to call DS_FreeString() on strings returned from this function.

[reuben]

Returning an allocated copy is unfortunate in this case because for getting the error string there are basically two main cases:

Case 1, abort entire program:

int err = DS_SomeAPI();
if (err != 0) {
    fprintf(stderr, "DeepSpeech error: %s\n", DS_ErrorCodeToErrorString(err));
    exit(1);
}

In this case freeing the string is pointless since you're exit-ing anyway.

Case 2, error propagation:

int err = DS_SomeAPI();
if (err != 0) {
    char* ds_copy = DS_ErrorCodeToErrorMessage(err);
    char* our_copy = strdup(ds_copy); // caller shouldn't have to know about DS API
    DS_FreeString(ds_copy);
    return MyErrorAbstraction(STT_ERROR, our_copy);
}

In this case, if we allocate the error string, we force applications to immediately copy it again with their own allocator, or we force them to include custom allocator/deleter support in their error abstraction, because otherwise some caller far away is going to have to know about DS_FreeString. Whereas a const C string can be propagated without this problem:

int err = DS_SomeAPI();
if (err != 0) {
    return MyErrorAbstraction(STT_ERROR, DS_ErrorCodeToErrorMessage(err));
}

[reuben]

Granted, this is also true for DS_Version where we went with that approach. Maybe consistency is worth the awkwardness cost, specially since most of our language bindings abstract this away.

[kdavis-mozilla]

Yeah it's not ideal.

But having this mixed API where some strings need to be freed with

DS_FreeString()

and some strings do not need to be freed with

DS_FreeString()

is also not ideal as it's confusing.

As for Case1, your example is true of very simply programs and applications, but for production integrations, think a GUI based application, you would never just exit if someone encountered a DS_ERR_INVALID_ALPHABET error. You'd "catch" the error and present them with a dialog to specify a correct alphabet file.

[imskr]

How should I proceed with DS_FreeString() in the DS_ErrorCodeToErrorMessage() ?

[reuben]

@imskr please return a copy of the string using strdup and change the documentation accordingly to mention that people should use DS_FreeString on the returned pointer.

[imskr]

Okay, thank you @reuben @kdavis-mozilla

[imskr]

Is it good now? @reuben

[reuben]

@imskr thanks for the PR!

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.