kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

On 04/29/2012 11:05 AM, Lorenzo Marcantonio wrote:
> On Sun, Apr 29, 2012 at 09:06:07AM -0500, Dick Hollenbeck wrote:
>> I don't know if you even want feedback.  This nice work.
> Feedback is always good.
>
>> I do have some minor points here, and then search for my initials below "RHH".
>>
>>
>> 1) Since it was such a significant revision, would have been nice to get the code more in
>> line with current coding standards, such as a) uppercase public function names, b) single
>> line comments C++ sytle, c) function comments in the header file, not in the C++ file. 
>> Remember that function comments are inherited by Doxygen into a polymorphic derivative
>> function.  So they only need to go into the base class within the header file unless there
>> is something different in a derivative.  Doxygen scans for functions in header files as a
>> preference to C++, so that is our preference.
> OK. Didn't knew about the doxygen bit.
>
>> 2) There were a couple of places where you removed KiROUND() when converting from double
>> to int.  Maybe you wanted a different rounding algorith, but you also removed
>> overflow/underflow detection which KiROUND() provides.
> I'll look into them. Didn't know about that KiROUND feature, too.
>
>> Of the two, I like b) better, and of course even better yet would be upper case first
>> character function name.
>  
> Returning a DPOINT conses ...erhmm... creates and destroys the object, a pointer or a reference doesn't; 

If you do this common idiom, I think you will find it is faster:


    DPOINT pt = user_convert..( wxpt );


Since the compiler is pretty clever in a situation like this, it has the choice of using
the copy constructor, and I am sure gcc has something like this also:


    http://msdn.microsoft.com/en-us/library/ms364057%28VS.80%29.aspx


Remember, there is intentionally no virtual function table pointer in this class, (I
deliberately avoided a virtual destructor so as to avoid a virtual function table
pointer), and its only two doubles.  Not hard to construct it.

I was way beyond this consideration when I stated my preference.  But use what you want.



> choosing between a pointer or a reference is (90% of the time) only an issue of style. If pointers are preferred, no problem here. 
>
>> 4) It would be an improvement to provide a clue in the "scaling factor" type variables
>> what the conversion is, and best would be to include "per" in that variable name.  eg.
>> du_per_iu or similar tells me "device units per internal units".  "aScalingFactor" or
>> "device_scale" tells me nothing, other than that I need to go study somewhere else, a
>> nuisance.
> Uhmm thats difficult because the device scale is from IU to whatever the engine uses... maybe something like ius_per_device_unit should do the trick
>
>> 5) I like the change to const reference for the wxPoint arguments.  const references are
>> fine, because the const means you have no intention to modify, so the concerns I mentioned
>> in 3) do not apply.
> Also the compiler likes them for a whole host of optimization and warning checking...
>
> Working on it. I'll post a single diff from trunk to the modified code.


You are da man.  Thank you for your valuable time.

Dick