kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxxxx>]

On 4/13/2012 3:24 PM, Dick Hollenbeck wrote:
> 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.

I guess the wxWidgets folks figured to take advantage of round() on
platforms that support it rather then reinvent the wheel but it does
seem like overkill because the their solution when round() is not
available would work in both cases.

> 
> 
> 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.

I think HAVE_ROUND is in setup.h which is created when you configure
wxWidgets.

> 
> 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.

I agree but I think it would be prudent to add an assertion to catch any
integer overflow issues while we hash out the nanometer internal units
issues.

Wayne

> 
> 
> Dick
> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>