kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

On 04/13/2012 02:03 PM, Dick Hollenbeck wrote:
> On 04/13/2012 12:00 PM, Wayne Stambaugh wrote:
>> On 4/13/2012 10:04 AM, Dick Hollenbeck wrote:
>>>> Also, you may want to
>>>> change Mils2iu to use wxRound() so it will work for negative values.
>>> wxRound()'s use of C lib's round() seems like overkill to me.
>>> Since the objective is to produce an integer, not a double, there is an easier way that
>>> has the possibility of letting the compiler do some of the work, i.e. in advance, on
>>> constants,
>>> without the overhead of a mandatory floating point function call.
>>>
>>> See my latest DMils2iu() as a suggestion.
>>>
>>>
>>> Dick
>> wxRound may be overkill


I did not say it was overkill.  I said it can be making an unnecessary function call.


Some of these are in static constructors.  Some are deferred in time, done in normal
constructors.


In any case, it can increase code size, maybe even delay startup if enough are in static
constructors.


If constants are passed to wxRound() then there is no reason for a runtime function call,
this kind of calculation can be done at compile time, resulting in the optimizer finding a
suitable integer constant that is put into the code at compile time, at least for the
Release build.  For the Debug build, this optimization will not happen especially if there
is an assert in our functions.  The compiler cannot do the reduction with the assert test
in play, the runtime has to evaluate that.

So the Debug build will catch the problems with constants, but then knowing there are
none, there is no reason to keep doing this runtime check *ON CONSTANTS*.


Look here:


    inline int wxRound(double x)
    {
        wxASSERT_MSG( x > INT_MIN - 0.5 && x < INT_MAX + 0.5,
                      wxT("argument out of supported range") );

        #if defined(HAVE_ROUND)
            return int(round(x));
        #else
            return (int)(x < 0 ? x - 0.5 : x + 0.5);
        #endif
    }



The call to round() should not happen.  The alternate path seems more than adequate to
me.  The assert is a good thing.


However, I do not see where HAVE_ROUND is defined.  So I do not know which code path is
taken in the above, FOR SURE.

There are a couple of issues at work here. 

I'm just saying "wxRound()'s use of round()", seems inappropriate, it should be avoided.

I would hope we can agree that 150 unnecessary function calls that are easily avoided
might be worth 5 to 10 lines of code.


Dick