kicad-developers team mailing list archive

[Lorenzo Marcantonio <l.marcantonio@xxxxxxxxxxxx>]

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

-- 
Lorenzo Marcantonio
Logos Srl