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