Re: [Gnash-dev] OpenVG support coming

[Benjamin Wolsey]

> Actually there have been very few interface changes, more moving code
> around and adding piles of new code. The libdevice internal API is new
> in it's entirety. I do plan to add more doxygen documentation as I do
> the merge, and you'll see the commits in the email list.

I can't currently compile this code because I don't have openvg on my
machines.

I took a look at random at OpenVGStyle.h
( 
http://git.savannah.gnu.org/cgit/gnash.git/tree/librender/openvg/OpenVGStyle.h?h=openvg
 ) and discovered that it hasn't really got the point of static visitors for a 
boost::variant. These are supposed to be designed to handle each type 
differently, much like virtual functions do. They aren't supposed to provide 
redundant getter-setters for each member of each variant type. The agg renderer 
can be used as a reference for how to do it properly.

Functions with a return type should actually return something (see line
167-168). I would hope the compiler warns about this when it's compiled.

I also noticed that, as a consequence of treating static_visitors like
extremely complicated getter-setters, GradientFill
( http://git.savannah.gnu.org/cgit/gnash.git/tree/libcore/FillStyle.h?h=openvg 
)now has a new, undocumented function that duplicates existing functionality:

> const GradientRecords &getRecords() const {
>         return _gradients;
>     }

I don't have any real problem with replacing the existing accessors
(which return the gradient stops by index), but I do have a problem with
undocumented and unnecessary additions to a simple API: this makes code
bases into a large mess (not to be confused with 'complex'). If the old
functions are to be replaced, code in the existing three renderers needs
to be adapted to the new accessor and the old accessors removed.

The position of the reference operator is also inconsistent with the
rest of Gnash.

In this file:

http://git.savannah.gnu.org/cgit/gnash.git/tree/librender/openvg/OpenVGBitmap.cpp?h=openvg

the function from lines 128:

> image::GnashImage&
> OpenVGBitmap::image()
> {
>     // GNASH_REPORT_FUNCTION;
>     if (_image) {
>         return *_image;
>     }
> }    

lacks an appropriate return. I'd expect the compiler to say "Warning:
control reaches end of non-void function" here.

I'm not going to review the code extensively because, as Sandro said,
it's currently not easy to see where the additions are. Before any
further reading of this code, it would also be nice if the Real
Programmer would fix at least the errors that his compiler is telling
him about.

bwy


-- 
--
Free Flash, use Gnash
http://www.gnu.org/software/gnash/

Benjamin Wolsey, Software Developer - http://benjaminwolsey.de
C++ and Open-Source Flash blog - http://www.benjaminwolsey.de/bwysblog

xmpp:address@hidden
http://identi.ca/bwy

signature.asc
Description: This is a digitally signed message part