On Mon, 2011-12-12 at 16:01 +0100, Peter Meerwald wrote:
> > Another thing is the question of when pa_strerror() can be used. That
> > should be documented unambiguously. I don't know what the current
> > situation is - my guess would be that the documentation is lacking in
> > this regard.
>
> I was not aware of this problem and simply assumed that an error code can
> be passed to pa_strerror()
That may be a correct assumption, but that's currently not said
explicitly anywhere, not even after your patch.
> > I think this text should either say what it says now, or that
> > pa_strerror() can be used always unless otherwise stated. Which one is
> > the right choice depends on which is more common, those error codes that
> > can be passed to pa_strerror() or those that can't (I haven't checked
> > what is the situation).
>
> /** Return a human readable error message for the specified numeric error code */
> const char* pa_strerror(int error);
>
> here no indication what a numeric error code is supposed to be; the API
> uses 'int' and not a 'pa_error_code_t' (which I suggest to introduce for a
> different reason)
>
> in def.h you find:
> /** Error values as used by pa_context_errno(). Use pa_strerror() to
> convert these values to human readable strings */
>
> strictly speaking, this would restrict pa_strerror() to values returned by
> pa_context_errno()
Indeed, that needs fixing.
> however, the example pacat-simple.c does the following:
> if (!(s = pa_simple_new(NULL, argv[0], PA_STREAM_PLAYBACK, NULL, "playback", &ss, NULL, NULL, &error))) {
> fprintf(stderr, __FILE__": pa_simple_new() failed: %s\n", pa_strerror(error));
>
> I have not spotted any error codes not defined in defs.h in a brief review
>
> > Regardless of what is the current situation, I think it would make sense
> > to guarantee every error code in the public API to be suitable to be
> > passed to pa_strerror().
>
> I fully agree, this sounds sensible
> I don't see a reason to have error codes as int and not enums -- but I
> guess that would be a too bit change at the moment
A couple of reasons come to mind (I don't know the original reasoning):
* Enum types should be avoided in public function signatures, because
the size of the type may, at least in theory, change when the enum is
extended, causing ABI breakage. (Yes, there are many places where enums
are currently used in function signatures in the public API. I don't
know why that is the case - possible reasons are not knowing this
problem, or forgetting it, or regarding it as a problem that doesn't
exist in practice.)
* In cases where a function can return either a negative or positive
number, using pa_error_code_t would not be the correct type for the
positive value (ie. not error). And actually, since pa_error_code_t
values are non-negative, it would be an incorrect type also for
functions that return the errors as negative numbers.
* It's a common pattern in C to use int as the error code type.
--
Tanu
