Re: ALSA C++ API updated for 1.0.16

From: Clemens Ladisch <clemens@ladisch.de>
To: "Lasse Kärkkäinen" <tronic@trn.iki.fi>
Cc: alsa-devel@alsa-project.org
Subject: Re: ALSA C++ API updated for 1.0.16
Date: Tue, 26 Feb 2008 11:01:43 +0100	[thread overview]
Message-ID: <47C3E387.20800@ladisch.de> (raw)
In-Reply-To: <47C30EDE.8000400@trn.iki.fi>

Lasse Kärkkäinen wrote:
> Here's an updated version of the C++ wrapper that I posted earlier in
> September 2007. I would like this to be included in the ALSA
> distribution after some peer review.

Yes, an ALSA C++ wrapper is a good idea.

> error(std::string const& function, int err): std::runtime_error("ALSA " + 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.

> pcm(char const* device = "default", snd_pcm_stream_t stream = SND_PCM_STREAM_PLAYBACK, int mode = 0) {

This looks too much like a default constructor.  I think neither device
nor stream should have a default value.

> ALSA_HPP_CLASS& set_##name(...) { ...; return *this; }\

Is there a reason why you return the object itself?  Usually, returning
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.

> class hw_config: internal::noncopyable {

Why should this object not be copyable?

And why do you have two different objects (*w_params and *w_config) for
wrapping the hardware/software parameters?

> 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 device
unplugged).

I think it would be better if this object doesn't have a public
constructor, and you would be able to create one only through a pcm
object, like "hw_config c = pcm.any_hw_params();".

> ~mmap() {
> 	// We just assume that this works (can't do anything sensible if it fails).
> 	snd_pcm_mmap_commit(pcm, offset, frames);

This is the place where all the usual write errors must be checked.
This cannot be done in a destructor.  I think using RAII for the non-
error commit just isn't possible.

Regards,
Clemens
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel