In the file libnr/nr-point.h, there are some C-Style casts in the code: /** \brief A function to lower the precision of the point \param places The number of decimal places that should be in the final number.
This function accomplishes its goal by doing a 10^places and multipling the X and Y coordinates by that. It then casts that to an integer (to get rid of all decimal points) and then divides by 10^places back again. */ inline void round (int places = 0) { _pt[X] = (Coord)(Inkscape::round((double)_pt[X] * pow(10, places)) / pow(10, places)); _pt[Y] = (Coord)(Inkscape::round((double)_pt[Y] * pow(10, places)) / pow(10, places)); return; }
Now casting _pt[] to a double is no-op, and possibly the (Coord) cast is also unnecessary, or could be converted to a function-style one: Coord( ... ) .
My problem centres on ensuring that the code matches the comment, which refers to casting to an int. I am fairly sure that the Inkscape::round( ) function actually is a wrapper around std::floor( ) which does everything in IEEE double precision.
IMHO, the Coord type should have a round( ) function, which I have had to provide under a different name:
namespace NR {
/** - * A "real" type with sufficient precision for coordinates. + * A floating point type with sufficient precision for coordinates. * * You may safely assume that double (or even float) provides enough precision for storing * on-canvas points, and hence that double provides enough precision for dot products of * differences of on-canvas points. */ -typedef double Coord; + typedef double Coord; + + /** + \brief A function to lower the precision of a co-ordinate + \param places The number of decimal places that should remain in + this co-ordinate. + + This function accomplishes its goal by multipling the co-ordinate by + (10^places). It then uses the Inkscape round( ) function (currently + implemented with std::floor( )) to remove the surplus precision and + then divides by (10^places) to scale back again. + */ + inline Coord set_precision (Coord c, int places = 0) { +// g_assert( places > 0 ); // Assert on do-nothing calls and negative number of decimal places + double factor = pow(10, places); + return Coord((Inkscape::round(c * factor) / factor));; + }
} /* namespace NR */
and then I can re-do the function as: /** \brief A function to lower the precision of the point \param places The number of decimal places that should remain in the co-ordinates of the point. */ inline void round (int places = 0) { // g_assert( places > 0 ); // Assert on do-nothing calls and negative number of decimal places _pt[X] = NR::set_precision(_pt[X], places); _pt[Y] = NR::set_precision(_pt[Y], places); return; }
Is this the way to go?
Would these changes be acceptable in any way?
Should Coord be a class rather than a typedef? Should the round( ) function(s) which are really floor( ) functions be renamed? Should the round functions(s) which are really set_precision( ) (if any) be renamed? Should any of these functions be templates e.g. (untested)
template <class T> inline void round( int places=0 ) { // round co-ordinates to places decimal places }
How are modifications to working code meant to be tested?
I would be happy to submit patches for review, or pursue this in any other way: I am trying to see whether it is viable to remove C-Style casts as per http://www.inkscape.org/cgi-bin/wiki.pl?InkscapeJanitors