kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

On 05/10/2012 12:05 PM, Dick Hollenbeck wrote:
> 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.

or

namespace PCB {

    enum LAYER
    {
    }

}

Then we can use "LAYER" in the code after a suitable

using namespace PCB;


Clarity is good, verbosity is bad, somewhere in the middle is a sweet spot.




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