Removing C-Style casts

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Removing C-Style casts

Ben Fowler
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


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. <a href="http://ads.osdn.com/?ad_idt77&alloc_id492&op=click">http://ads.osdn.com/?ad_idt77&alloc_id492&op=click
_______________________________________________
Inkscape-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/inkscape-devel
Reply | Threaded
Open this post in threaded view
|

Fwd: Removing C-Style casts

Ben Fowler
Not sure that I have a complere grasp of this web thingy

---------- Forwarded message ----------
From: Ben Fowler <[hidden email]>
Date: Jul 2, 2005 4:18 PM
Subject: Removing C-Style casts
To: [hidden email]


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


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. <a href="http://ads.osdn.com/?ad_idt77&alloc_id492&op=click">http://ads.osdn.com/?ad_idt77&alloc_id492&op=click
_______________________________________________
Inkscape-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/inkscape-devel
Reply | Threaded
Open this post in threaded view
|

Fwd: Removing C-Style casts

Ben Fowler
In reply to this post by Ben Fowler
Not sure that I have a complete grasp of this web thingy

---------- Forwarded message ----------
From: Ben Fowler <[hidden email]>
Date: Jul 2, 2005 4:18 PM
Subject: Removing C-Style casts
To: [hidden email]


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


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. <a href="http://ads.osdn.com/?ad_idt77&alloc_id492&op=click">http://ads.osdn.com/?ad_idt77&alloc_id492&op=click
_______________________________________________
Inkscape-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/inkscape-devel
Reply | Threaded
Open this post in threaded view
|

Fwd: Removing C-Style casts

Ben Fowler
In reply to this post by Ben Fowler
Not sure that I have a complete grasp of this web thingy

---------- Forwarded message ----------
From: Ben Fowler <[hidden email]>
Date: Jul 2, 2005 4:18 PM
Subject: Removing C-Style casts
To: [hidden email]


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


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. <a href="http://ads.osdn.com/?ad_idt77&alloc_id492&op=click">http://ads.osdn.com/?ad_idt77&alloc_id492&op=click
_______________________________________________
Inkscape-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/inkscape-devel