[vlc-devel] commit: New C++ binding for libvlc. ( R?mi Duraffort )

[jpd at videolan.org]

On Sat, Jan 23, 2010 at 05:10:36PM +0100, R?mi Duraffort wrote:
> > Awesome! I think you should drop the get prefix for the getters.
> 
> Like:
> MediaPlayer::time() for the getter
> MediaPlayer::setTime() for the setter ?

You don't even need a set prefix, overloading will take care of it:

mtime_t MediaPlayer::time();
void MediaPlayer::time(mtime_t);

A couple more points:

> +Exception::~Exception()
> +{
> +    if( libvlc_exception_raised( &ex ) )
> +        throw libvlc_errmsg();
> +    libvlc_exception_clear( &ex );
> +}

The libvlc_exception_clear() call is clearly superfluous here. Throwing
an exception causes a stack rewind and so you'll need to clear the
exception at the point the exception is caught, and otherwise it
doesn't need clearing. I think it would be a better idea to throw a
libvlc::Exception instead. And perhaps add some methods to it.

I also think that the wrapper classes should inline (much) more
methods; now they're convenient but add entirely avoidable call
overhead. And finally I think that the Exception instances in the
wrapper methods should be replaced by a single member, to save a ton of
libvlc_exception_init() calls (and libvlc_exception_clear() calls) that
are strictly unnecessairy. I mean, that's five call instructions plus
argument passing where previously we'd've used two, for each method.

Otherwise, it's a reasonable start.


As an aside, reading the docu-comments I see a lot of words that aren't
adding much value. Simple things like ``Get the agl handler associated
with the media player'' describing what is clearly a getter method
inside the media player class, are pretty much describing the obvious
but what is an agl handler and why do I need it? The term hasn't been
introduced and there's no reference where to find out more. This
shouldn't be a tick-box item but should be written with a perspective
to explain the prospective user not what it does but what it is there
for and how to use it. This is a general problem with docu-comments, but
should nonetheless be fixed.