kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

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