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
_______________________________________________
Mailing list: https://launchpad.net/~kicad-developers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~kicad-developers
More help : https://help.launchpad.net/ListHelp
