[vlc-devel] commit: New C++ binding for libvlc. ( R?mi Duraffort )
jpd at videolan.org
jpd at videolan.org
Mon Jan 25 09:48:50 CET 2010
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.
> MediaPlayer::time() for the getter
> MediaPlayer::setTime() for the setter ?
You don't even need a set prefix, overloading will take care of it:
A couple more points:
> + 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.
More information about the vlc-devel