kicad-developers team mailing list archive

[Maciej Sumiński <maciej.suminski@xxxxxxx>]

I have briefly tested the patch, no issues found. I have no objections
to the changes, but I will leave the final decision to Wayne.

Regards,
Orson

On 03/22/2017 03:51 AM, Jon Evans wrote:
> Hi Wayne, new patch attached.
> 
> Thanks,
> Jon
> 
> On Tue, Mar 21, 2017 at 9:28 AM, Wayne Stambaugh <stambaughw@xxxxxxxxx>
> wrote:
> 
>> Jon,
>>
>> I just attempted to apply your patch and it no longer applies cleanly.
>> Please rebase it when you get a chance.
>>
>> Thanks,
>>
>> Wayne
>>
>> On 3/16/2017 10:14 AM, Jon Evans wrote:
>>> Bump -- does anyone have time to look at this and report back if there
>>> are any issues?  I'd like to get it merged so that I can feel confident
>>> about doing more work on top of these changes.
>>>
>>> Thanks,
>>> Jon
>>>
>>> On Tue, Mar 14, 2017 at 6:05 PM, Wayne Stambaugh <stambaughw@xxxxxxxxx
>>> <mailto:stambaughw@xxxxxxxxx>> wrote:
>>>
>>>     Hey Jon,
>>>
>>>     This is better than the giant enum concept and I'm willing to accept
>>>     this.  It still lacks the type safety of the enum inheritance
>> solution.
>>>     I still see ints being cast to enums and enum bounds checking so
>> this is
>>>     less than ideal.  I would prefer to see some additional testing so if
>>>     any one has time, please test this patch before we commit it.
>>>
>>>     Thanks,
>>>
>>>     Wayne
>>>
>>>     On 3/14/2017 3:09 PM, Jon Evans wrote:
>>>     > Hi Wayne,
>>>     >
>>>     > New patch attached.  Let me know what you think of this approach.
>>>     >
>>>     > Thanks,
>>>     > Jon
>>>     >
>>>     > On Tue, Mar 14, 2017 at 10:40 AM, Jon Evans <jon@xxxxxxxxxxxxx
>> <mailto:jon@xxxxxxxxxxxxx>
>>>     > <mailto:jon@xxxxxxxxxxxxx <mailto:jon@xxxxxxxxxxxxx>>> wrote:
>>>     >
>>>     >     Hi John, that's basically what my first patch did, but inside
>>>     one enum.
>>>     >
>>>     >     Hi Wayne, thanks for elaborating more, I see your point.
>>>     >
>>>     >     I am still not sure there is benefit to adding some class to
>>>     handle
>>>     >     enum inheritance.
>>>     >     I think it would work fine to just chain multiple enums
>>>     together, as
>>>     >     was done before, but with some tweaks.
>>>     >
>>>     >     enum PCB_LAYER_ID
>>>     >     {
>>>     >         F_Cu = 0,
>>>     >         //...
>>>     >         PCB_LAYER_ID_COUNT
>>>     >     };
>>>     >
>>>     >     enum NETNAME_LAYER_ID
>>>     >     {
>>>     >         NETNAME_LAYER_ID_START = PCB_LAYER_ID_COUNT,
>>>     >         NETNAME_LAYER_ID_COUNT = NETNAME_LAYER_ID_START +
>>>     PCB_LAYER_ID_COUNT
>>>     >     };
>>>     >
>>>     >     enum GAL_LAYER_ID
>>>     >     {
>>>     >         GAL_LAYER_ID_START = NETNAME_LAYER_ID_COUNT,
>>>     >         //....
>>>     >     };
>>>     >
>>>     >     And so on for gerbview, eeschema, etc
>>>     >
>>>     >     That way the IDs will be unique and cover a contiguous range,
>> so
>>>     >     functions that want to take any layer ID can just check that
>>>     the ID
>>>     >     is >= 0 and < the end sentinel of the last enum.
>>>     >
>>>     >     Any concerns with this approach?
>>>     >
>>>     >
>>>     >     Best,
>>>     >     Jon
>>>     >
>>>     >     On Tue, Mar 14, 2017 at 10:29 AM, John Beard
>>>     <john.j.beard@xxxxxxxxx <mailto:john.j.beard@xxxxxxxxx>
>>>     >     <mailto:john.j.beard@xxxxxxxxx
>>>     <mailto:john.j.beard@xxxxxxxxx>>> wrote:
>>>     >
>>>     >         Hi Jon,
>>>     >
>>>     >         Protocol Buffers has the same problem. Messages have a
>>>     unique number
>>>     >         for each field, but extensions to messages don't know about
>>>     >         "siblings". A common thing [1] to so is reserve IDs up to
>> 1000
>>>     >         for the
>>>     >         base message, and then messages start at offsets 1000,
>>>     2000, etc,
>>>     >         based on some in-house scheme.
>>>     >
>>>     >         It probably won't just "drop in" to KiCad due to fixed
>>>     arrays (I
>>>     >         think?), but this is certainly one way it has been
>>>     "solved" in the
>>>     >         real world, by Google, no less! It's a bit crusty to
>> manually
>>>     >         reserve
>>>     >         space, but the "enum inheritance" problem isn't limited to
>>>     C++.
>>>     >
>>>     >         There's plenty of space in enums and I sincerely doubt
>>>     there is a
>>>     >         measurable benefit to forcing the compiler to use shorter
>>>     integral
>>>     >         types anyway, so we could just say "0-9999" is "common
>> GAL",
>>>     >         "10000-19999" is Pcbnew, etc. Some work might be needed to
>>>     handle
>>>     >         non-contiguous enums here. "10000 layers should be enough
>> for
>>>     >         anyone",
>>>     >         right?
>>>     >
>>>     >         Just a thought, without any real consideration of the
>>>     >         consequences for
>>>     >         things like formats and so on.
>>>     >
>>>     >         Cheers,
>>>     >
>>>     >         John
>>>     >
>>>     >         [1]
>>>     >
>>>      https://developers.google.com/protocol-buffers/docs/proto#
>> choosing-extension-numbers
>>>     <https://developers.google.com/protocol-buffers/docs/
>> proto#choosing-extension-numbers>
>>>     >
>>>      <https://developers.google.com/protocol-buffers/docs/
>> proto#choosing-extension-numbers
>>>     <https://developers.google.com/protocol-buffers/docs/
>> proto#choosing-extension-numbers>>
>>>     >
>>>     >         On Tue, Mar 14, 2017 at 10:08 PM, Jon Evans
>>>     <jon@xxxxxxxxxxxxx <mailto:jon@xxxxxxxxxxxxx>
>>>     >         <mailto:jon@xxxxxxxxxxxxx <mailto:jon@xxxxxxxxxxxxx>>>
>> wrote:
>>>     >         > Hi Orson, Wayne,
>>>     >         >
>>>     >         > I looked at the "enum inheritance" thing some more and I
>>>     don't
>>>     >         think it
>>>     >         > would be a good solution for our case.
>>>     >         >
>>>     >         > This technique lets you extend enum A with enum B, and
>> have
>>>     >         functions f(A)
>>>     >         > and f(A or B), and you could continue making larger
>>>     enums that
>>>     >         contained
>>>     >         > smaller ones.
>>>     >         > But, if we maintain multiple enums (one for each
>>>     application,
>>>     >         plus one for
>>>     >         > GAL layers) I don't see how it would make anything
>> simpler,
>>>     >         because we would
>>>     >         > not be able to treat them as "sibling classes"
>>>     >         >
>>>     >         > Before I spend more time coding things I want to get an
>> idea
>>>     >         of what your
>>>     >         > requirements are / what you would and would not accept
>> as a
>>>     >         change in this
>>>     >         > area.  I misunderstood Wayne's earlier reply to me and
>>>     thought
>>>     >         that a single
>>>     >         > enum would be accepted, but if not, I don't currently
>>>     have a good
>>>     >         > understanding of what the concerns are with that
>> approach.
>>>     >         >
>>>     >         > Questions for Wayne, Orson, and others who care about
>> this:
>>>     >         >
>>>     >         > 1) Is there any opposition to moving the layer
>> definitions
>>>     >         from GerbView and
>>>     >         > Eeschema into layers_id_colors_and_visibility.h?
>> (ignoring
>>>     >         whether they are
>>>     >         > merged into one enum for now)
>>>     >         >
>>>     >         > 2) Is there any opposition to ensuring that no layer IDs
>>>     >         overlap across all
>>>     >         > applications?  To be clear, what I mean now is that
>>>     currently
>>>     >         GerbView draw
>>>     >         > layers occupy the same layer IDs as Pcbnew board
>> layers.  I
>>>     >         want to change
>>>     >         > it so that a layer ID (cast to integer) is always unique
>>>     >         across all
>>>     >         > applications, unless it truly is the same layer (i.e can
>> use
>>>     >         the same color
>>>     >         > settings, visibility settings, etc. as GP_OVERLAY can
>> across
>>>     >         > GerbView/Pcbnew).
>>>     >         >
>>>     >         > 3) If the answers to both 1 and 2 are "no", can you give
>>>     some
>>>     >         more details
>>>     >         > on why it's a bad idea to put all the layers in the same
>>>     enum,
>>>     >         and based on
>>>     >         > that I will come back with a proposal on a different way
>> of
>>>     >         doing it?
>>>     >         >
>>>     >         > Thanks,
>>>     >         > Jon
>>>     >         >
>>>     >         > On Mon, Mar 13, 2017 at 3:07 PM, Jon Evans
>>>     <jon@xxxxxxxxxxxxx <mailto:jon@xxxxxxxxxxxxx>
>>>     >         <mailto:jon@xxxxxxxxxxxxx <mailto:jon@xxxxxxxxxxxxx>>>
>> wrote:
>>>     >         >>
>>>     >         >> Hi Orson,
>>>     >         >>
>>>     >         >> It's an interesting idea, I will have to look at it
>> more.
>>>     >         But, doesn't
>>>     >         >> this still allow the programmer to accidentally overlap
>> two
>>>     >         enum values?  I
>>>     >         >> can add checks to prevent this from happening elsewhere
>> in
>>>     >         the code, but it
>>>     >         >> seems less clean to me.
>>>     >         >>
>>>     >         >> Best,
>>>     >         >> -Jon
>>>     >         >>
>>>     >         >> On Mon, Mar 13, 2017 at 1:52 PM, Maciej Suminski
>>>     >         <maciej.suminski@xxxxxxx <mailto:maciej.suminski@xxxxxxx>
>>>     <mailto:maciej.suminski@xxxxxxx <mailto:maciej.suminski@xxxxxxx>>>
>>>     >         >> wrote:
>>>     >         >>>
>>>     >         >>> Hi,
>>>     >         >>>
>>>     >         >>> How about emulating enum inheritance [1]? I suppose it
>>>     would
>>>     >         be the
>>>     >         >>> cleanest solution allowing us to clearly specify what
>> kind
>>>     >         of layer is
>>>     >         >>> expected for certain methods. This is even better kind
>> of
>>>     >         type checking.
>>>     >         >>>
>>>     >         >>> Cheers,
>>>     >         >>> Orson
>>>     >         >>>
>>>     >         >>> 1. https://www.codeproject.com/kb/cpp/inheritenum.aspx
>>>     <https://www.codeproject.com/kb/cpp/inheritenum.aspx>
>>>     >         <https://www.codeproject.com/kb/cpp/inheritenum.aspx
>>>     <https://www.codeproject.com/kb/cpp/inheritenum.aspx>>
>>>     >         >>>
>>>     >         >>> On 03/13/2017 02:50 PM, Jon Evans wrote:
>>>     >         >>> > Hi Wayne,
>>>     >         >>> >
>>>     >         >>> > I understand this might seem like too big a change.
>>>     >         >>> > Here is what I was thinking when I thought that
>>>     combining
>>>     >         everything
>>>     >         >>> > would
>>>     >         >>> > be a good solution.
>>>     >         >>> >
>>>     >         >>> > - If there is more than one enum, then functions that
>>>     >         consume data from
>>>     >         >>> > more than one app (i.e. things in common/GAL) have
>>>     to cast
>>>     >         to int, so
>>>     >         >>> > you
>>>     >         >>> > lose type checking that the enum gives you for free
>> (or
>>>     >         your type
>>>     >         >>> > checking
>>>     >         >>> > gets more complicated, because the range of valid
>> values
>>>     >         is different
>>>     >         >>> > for
>>>     >         >>> > each application)
>>>     >         >>> >
>>>     >         >>> > - If there is more than one enum, it's easier to
>>>     duplicate
>>>     >         layers for
>>>     >         >>> > no
>>>     >         >>> > good reason (i.e. GerbView and Pcbnew have different
>>>     layer
>>>     >         ids for
>>>     >         >>> > "grid"
>>>     >         >>> > right now)
>>>     >         >>> >
>>>     >         >>> > - I want to combine the color settings for all
>>>     >         applications under the
>>>     >         >>> > hood
>>>     >         >>> > (users will still be able to configure different
>> colors
>>>     >         for each
>>>     >         >>> > application).  This change will let color settings
>> take
>>>     >         LAYER_ID
>>>     >         >>> > instead of
>>>     >         >>> > int, and there will only be one key/value mapping of
>>>     >         colors -- no more
>>>     >         >>> > difference between "GetLayerColor" and
>> "GetItemColor".
>>>     >         There will be
>>>     >         >>> > no
>>>     >         >>> > clashes between the meaning of a layer id (int type)
>>>     >         between different
>>>     >         >>> > applications.
>>>     >         >>> >
>>>     >         >>> > - Bringing Eeschema into this now is just early
>>>     groundwork
>>>     >         for Eeschema
>>>     >         >>> > GAL
>>>     >         >>> > port (as well as unified color settings)
>>>     >         >>> >
>>>     >         >>> > If you will not accept this change, I have to think
>>>     about
>>>     >         a different
>>>     >         >>> > proposal that will make the different layer types in
>>>     different
>>>     >         >>> > applications
>>>     >         >>> > a bit more manageable than they are today.  I
>> understand
>>>     >         how having one
>>>     >         >>> > giant enum for LAYER_ID seems more complicated, I'm
>> just
>>>     >         worried that
>>>     >         >>> > having several different enums will make the code
>> that
>>>     >         consumes
>>>     >         >>> > LAYER_ID
>>>     >         >>> > more complicated, especially if the applications
>> become
>>>     >         more integrated
>>>     >         >>> > with each other and start sharing more code.
>>>     >         >>> >
>>>     >         >>> > Best,
>>>     >         >>> > Jon
>>>     >         >>> >
>>>     >         >>> > On Mon, Mar 13, 2017 at 8:21 AM, Wayne Stambaugh
>>>     >         <stambaughw@xxxxxxxxx <mailto:stambaughw@xxxxxxxxx>
>>>     <mailto:stambaughw@xxxxxxxxx <mailto:stambaughw@xxxxxxxxx>>>
>>>     >         >>> > wrote:
>>>     >         >>> >
>>>     >         >>> >> Jon,
>>>     >         >>> >>
>>>     >         >>> >> I misunderstood your original intent.  I don't think
>>>     >         cluttering the
>>>     >         >>> >> board layer enums with all of the virtual layer and
>>>     >         schematic layer
>>>     >         >>> >> enums is a good idea.  It just seems like overkill
>> to
>>>     >         me.  I thought
>>>     >         >>> >> you
>>>     >         >>> >> were going to create a separate enum for virtual
>> board
>>>     >         layers.  You
>>>     >         >>> >> could always start the virtual board layer enums
>>>     from the
>>>     >         last board
>>>     >         >>> >> layer enum if you need unique enums.  I would also
>>>     prefer the
>>>     >         >>> >> schematic
>>>     >         >>> >> layer enums be separate from the board layer enums
>> for
>>>     >         clarity.
>>>     >         >>> >> Anyone
>>>     >         >>> >> else have any thoughts on this?
>>>     >         >>> >>
>>>     >         >>> >> Cheers,
>>>     >         >>> >>
>>>     >         >>> >> Wayne
>>>     >         >>> >>
>>>     >         >>> >> On 3/12/2017 11:24 PM, Jon Evans wrote:
>>>     >         >>> >>> Hi all,
>>>     >         >>> >>>
>>>     >         >>> >>> Per the other thread, this patch unifies the layer
>>>     >         definitions
>>>     >         >>> >>> between
>>>     >         >>> >>> Pcbnew, GerbView, and Eeschema.  It removes the
>>>     need for
>>>     >         >>> >>> ITEM_GAL_LAYER
>>>     >         >>> >>> and some other macros, and it will simplify the
>>>     >         implementation of
>>>     >         >>> >>> cross-application color themes and using GAL in
>>>     multiple
>>>     >         >>> >>> applications.
>>>     >         >>> >>>
>>>     >         >>> >>> Note that this patch introduces some temporary
>>>     weirdness
>>>     >         in a few
>>>     >         >>> >>> places, such as in COLORS_DESIGN_SETTINGS (there
>>>     is now
>>>     >         a single
>>>     >         >>> >>> array
>>>     >         >>> >>> for color storage, but it's still referred to by
>>>     two sets of
>>>     >         >>> >>> getters/setters).  This is because I wanted to
>>>     keep this
>>>     >         refactor as
>>>     >         >>> >>> simple as possible, as I plan to follow it up with
>> an
>>>     >         overhaul of
>>>     >         >>> >>> color
>>>     >         >>> >>> settings when I share my color themes work.  I
>> didn't
>>>     >         want to do work
>>>     >         >>> >>> that I would soon end up getting rid of anyway.
>>>     >         >>> >>>
>>>     >         >>> >>> Best,
>>>     >         >>> >>> Jon
>>>     >         >>> >>>
>>>     >         >>> >>>
>>>     >         >>> >>> _______________________________________________
>>>     >         >>> >>> Mailing list:
>>>     https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>
>>>     >         <https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>>
>>>     >         >>> >>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>     >         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>>
>>>     >         >>> >>> Unsubscribe : https://launchpad.net/~kicad-
>> developers
>>>     <https://launchpad.net/~kicad-developers>
>>>     >         <https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>>
>>>     >         >>> >>> More help   : https://help.launchpad.net/ListHelp
>>>     <https://help.launchpad.net/ListHelp>
>>>     >         <https://help.launchpad.net/ListHelp
>>>     <https://help.launchpad.net/ListHelp>>
>>>     >         >>> >>>
>>>     >         >>> >>
>>>     >         >>> >> _______________________________________________
>>>     >         >>> >> Mailing list: https://launchpad.net/~kicad-
>> developers
>>>     <https://launchpad.net/~kicad-developers>
>>>     >         <https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>>
>>>     >         >>> >> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>     >         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>>
>>>     >         >>> >> Unsubscribe : https://launchpad.net/~kicad-
>> developers
>>>     <https://launchpad.net/~kicad-developers>
>>>     >         <https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>>
>>>     >         >>> >> More help   : https://help.launchpad.net/ListHelp
>>>     <https://help.launchpad.net/ListHelp>
>>>     >         <https://help.launchpad.net/ListHelp
>>>     <https://help.launchpad.net/ListHelp>>
>>>     >         >>> >>
>>>     >         >>> >
>>>     >         >>> >
>>>     >         >>> >
>>>     >         >>> > _______________________________________________
>>>     >         >>> > Mailing list: https://launchpad.net/~kicad-
>> developers
>>>     <https://launchpad.net/~kicad-developers>
>>>     >         <https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>>
>>>     >         >>> > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>     >         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>>
>>>     >         >>> > Unsubscribe : https://launchpad.net/~kicad-
>> developers
>>>     <https://launchpad.net/~kicad-developers>
>>>     >         <https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>>
>>>     >         >>> > More help   : https://help.launchpad.net/ListHelp
>>>     <https://help.launchpad.net/ListHelp>
>>>     >         <https://help.launchpad.net/ListHelp
>>>     <https://help.launchpad.net/ListHelp>>
>>>     >         >>> >
>>>     >         >>>
>>>     >         >>> _______________________________________________
>>>     >         >>> Mailing list: https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>
>>>     >         <https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>>
>>>     >         >>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>     >         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>>
>>>     >         >>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>
>>>     >         <https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>>
>>>     >         >>> More help   : https://help.launchpad.net/ListHelp
>>>     <https://help.launchpad.net/ListHelp>
>>>     >         <https://help.launchpad.net/ListHelp
>>>     <https://help.launchpad.net/ListHelp>>
>>>     >         >>
>>>     >         >>
>>>     >         >
>>>     >         >
>>>     >         > _______________________________________________
>>>     >         > Mailing list: https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>
>>>     >         <https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>>
>>>     >         > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>     >         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>>
>>>     >         > Unsubscribe : https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>
>>>     >         <https://launchpad.net/~kicad-developers
>>>     <https://launchpad.net/~kicad-developers>>
>>>     >         > More help   : https://help.launchpad.net/ListHelp
>>>     <https://help.launchpad.net/ListHelp>
>>>     >         <https://help.launchpad.net/ListHelp
>>>     <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
>

Attachment: signature.asc
Description: OpenPGP digital signature