[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Gnash-dev] OpenVG support coming

From: Benjamin Wolsey
Subject: Re: [Gnash-dev] OpenVG support coming
Date: Tue, 22 Mar 2011 18:51:07 +0100

> 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

I took a look at random at OpenVGStyle.h
 ) 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
)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:

the function from lines 128:

> image::GnashImage&
> OpenVGBitmap::image()
> {
>     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.


Free Flash, use Gnash

Benjamin Wolsey, Software Developer -
C++ and Open-Source Flash blog -


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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]