On 05/09/2012 06:42 PM, Wayne Stambaugh wrote: > Lorenzo, > > I like a lot of the work you did here and I appreciate your efforts and > enthusiasm. I have a few comments about this patch. > > 1) It is fine for folks to test but there are too many changes at least > for me to review and commit at one time. For committing purposes I > would prefer this be broken into four separate patches. One each for > the EDA_COLOR_T, EDA_DRAW_MODE_T, PCB_LAYER_MASK, and the Eeshema > m_Layer removal. It's not necessarily the size of the patch. It's the > number of things it changes that makes me take pause. > > 2) Please convert tabs to 4 spaces. > > 3) Please remove trailing white space.
spaces and tabs, in particular. bzr will handle the end of line just fine, as needed, per platform. The name PCB_LAYER_NUMBER is a lot longer than int and I don't like it. Maybe LAYER_ID is a compromise, that is sufficiently descriptive with minimum verbosity, and does not tie too much into number. Wayne, I agree with you that this patch is too big, and thanks for saying it, and being the first to comment. Nowadays, if a patch takes more than 10 minutes to look at, it is too big for me. Dick > > 4) It appears you are using Windows as your development platform because > the new files you added have ^M as the line ending. Your editor should > be able to strip these out and configured not to use them. > > I'm extremely busy right now so I don't have any free time to test this. > Hopefully there are some other folks out there who can help out with > the testing. Thanks again for your efforts. > > Wayne > > On 5/9/2012 5:35 AM, Lorenzo Marcantonio wrote: >> The witch hunt gave its fruits. The patch is a biggie, now I'm testing >> it but obviously some bug could have crept it (aka the 'shit happens' >> principle). >> >> This would contribute to the C++-ification of kicad and also remove some >> slight nearly-impossible-to-hit bugs (bonus point: what's the difference >> between FIRST_NO_COPPER_LAYER and FIRST_NON_COPPER_LAYER? now the >> compiler tags the error). >> >> What happened: >> >> - On the exterior *nothing*, I hope. It's only an innard rework. >> >> - PCB/gerber layers representation changed from int to enum (and >> corresponding operators added). Newer g++ flags as error the assigment >> of an int to and enum! So it's more like a 'lightweight' class... >> We now have the PCB_LAYER_NUMBER and the PCB_LAYER_MASK. The >> GetLayerMask() was put inline in the header so we'll never seen the >> ugly (1 << layer) construct. PCB_LAYER_MASK has the appropriate >> boolean operators so they're handy to manipulate. Bonus: it's a big >> step for adding new layers since these are some big abstractions >> anyway (at least they flag every place where a layer/mask is used). >> Calls to LayerFromInt and static_cast mark 'difficult' places. >> >> - Many #defines converted to static const; that's the C++ style >> >> - The same enum-conversion was applied to colors. Now colors are not >> anymore anonymous ints but the EDA_COLOR_T is fully employed. As >> someone already know EDA_COLOR_T also carries some flags and the alpha >> should value in the higher bits. Some functions help encapsulate this. >> Also used the MakeColour when possible instead of accessing the raw >> rgb fields. >> >> - GRDrawMode encapsulated as an enum, too (it's a bit field struct, >> anyway, but enums help the compiler keeping them separate). >> >> - Completely removed m_Layer from the SCH_ITEM classes; now these are >> identified by the class only (i.e. no more subtype for lines which >> before could represent wires, buses or even graphic lines). The >> refactored classes were the line and bus entry. A factory load >> function is used to create the right class depending on the input >> record. >> >> - Removed SCH_POLYLINE since it was never implemented anyway >> >> - Some global renamed with the proper prefix (eg. Drc_On -> g_Drc_on) >> >> (still have to find the source for the 'same color of the background' >> message, anyway...) >> >> >> >> >> _______________________________________________ >> Mailing list: https://launchpad.net/~kicad-developers >> Post to : [email protected] >> Unsubscribe : https://launchpad.net/~kicad-developers >> More help : https://help.launchpad.net/ListHelp > _______________________________________________ > Mailing list: https://launchpad.net/~kicad-developers > Post to : [email protected] > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp > _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
