On Wed, Feb 27, 2008 at 8:50 AM, Clemens Ladisch clemens@ladisch.de wrote= :
Lasse K=E4rkk=E4inen wrote:
error(std::string const& function, int err): std::runtime_error("A=
LSA " + function + " failed: " + std::string(snd_strerror(err))), err(err) = {}
I'm not sure that including the function name and all parameters as =
they
appear in the source file in the error message is a good idea; I wouldn't want to impose this policy on all applications using this header.
It is just an exception thrown, the program may catch it and display another error message. The original ALSA error code is also included within it.
I'm just objecting to the fact that the function name and ALSA's error string are combined into a string that cannot easily be parsed later.
Many of those strings are not meant to be parsed, they are for human review (developer/user).
In practice the function name seems to be quite essential for debuggin=
g,
as EPIPE or some other of the standard error codes will tell little or nothing.
Agreed.
I would prefer that the method from the C++ API should appear instead of the internal C API function. The C++ developer is calling the constructor of alsa::pcm, not snd_pcm_open().
For completeness, I think that it might be desirable to also store the function name separately (like the code is stored), but can anyone see any actual use for this?
I'd like to have only the snd_strerror() result as the message. In this case, the function name has to be stored separately if it is needed.
I've found the snd_strerror() messages are quite unusable, at least for the alsaseq api... they return something like 'file not found' which seems more related to the internal implementation rather that an actual alsa-related error...
pcm(char const* device =3D "default", snd_pcm_stream_t stream =3D =
SND_PCM_STREAM_PLAYBACK, int mode =3D 0) {
This looks too much like a default constructor. I think neither dev=
ice
nor stream should have a default value.
Well, it is a default constructor. One design goal was to make this wrapper as easy to use as possible, and choosing sensible defaults for that is helpful.
While playback will be used more often, I think the difference between playback and capture is so big that we cannot make one of them the default.
Me too.
[...]
ALSA_HPP_CLASS& set_##name(...) { ...; return *this; }\
Is there a reason why you return the object itself? Usually, return=
ing
an object implies that that object is a _different_ object with the setting applied, which would mean that the original object was _not_ changed, but that isn't true here.
This is for function call chaining. It is a common idiom used by the standard library and others.
The STL streams use this to allow using the convert-to-bool operator to check the error status of the stream (because most errors aren't reported by exceptions). That this also allows chaining is just a side effect.
I found this feature quite useful.
hw_config(snd_pcm_t* pcm): pcm(pcm) { try { current(); } catch (std::runtime_error&) { any(); }
I don't like using exceptions when there's nothing wrong. Besides, getting the current parameters may fail due to other errors (like de=
vice
unplugged).
It seems like a sensible fallback, anyway. The other option would be t=
o
load any() settings always and have the user call current(), if he wan=
ts
those instead. Would you find this preferable?
My preference would be that the user has to indicate whether he wants to read the current setting or to begin choosing new settings. It is not possible for this constructor to guess what the user wants.
_From a OO perspective, I would expect that a *_config object be created from an existing pcm object. Something like:
alsa::pcm pcm;
pcm.get_config().set(FOO).set(BAR);
If there is something that doesn't apply to a pcm instance, the C++ API could provide an instance of a pcm_any object and call that instead:
alsa::pcm_any.get_config().set(FOO).set(BAR);
[...]
HTH,
--=20 Aldrin Martoq