← Back to team overview

kicad-developers team mailing list archive

Re: [PATCH] Refactor LAYER_ID to be the one and only layer definition

 

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>> 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>> 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>
> 
>         On Tue, Mar 14, 2017 at 10:08 PM, Jon Evans <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>> 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>>
>         >> 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>
>         >>>
>         >>> 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>>
>         >>> > 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>
>         >>> >>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>         >>> >>> Unsubscribe : https://launchpad.net/~kicad-developers
>         <https://launchpad.net/~kicad-developers>
>         >>> >>> More help   : https://help.launchpad.net/ListHelp
>         <https://help.launchpad.net/ListHelp>
>         >>> >>>
>         >>> >>
>         >>> >> _______________________________________________
>         >>> >> Mailing list: https://launchpad.net/~kicad-developers
>         <https://launchpad.net/~kicad-developers>
>         >>> >> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>         >>> >> Unsubscribe : https://launchpad.net/~kicad-developers
>         <https://launchpad.net/~kicad-developers>
>         >>> >> More help   : https://help.launchpad.net/ListHelp
>         <https://help.launchpad.net/ListHelp>
>         >>> >>
>         >>> >
>         >>> >
>         >>> >
>         >>> > _______________________________________________
>         >>> > Mailing list: https://launchpad.net/~kicad-developers
>         <https://launchpad.net/~kicad-developers>
>         >>> > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>         >>> > Unsubscribe : https://launchpad.net/~kicad-developers
>         <https://launchpad.net/~kicad-developers>
>         >>> > More help   : https://help.launchpad.net/ListHelp
>         <https://help.launchpad.net/ListHelp>
>         >>> >
>         >>>
>         >>> _______________________________________________
>         >>> Mailing list: https://launchpad.net/~kicad-developers
>         <https://launchpad.net/~kicad-developers>
>         >>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>         >>> Unsubscribe : https://launchpad.net/~kicad-developers
>         <https://launchpad.net/~kicad-developers>
>         >>> More help   : https://help.launchpad.net/ListHelp
>         <https://help.launchpad.net/ListHelp>
>         >>
>         >>
>         >
>         >
>         > _______________________________________________
>         > Mailing list: https://launchpad.net/~kicad-developers
>         <https://launchpad.net/~kicad-developers>
>         > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>         > Unsubscribe : https://launchpad.net/~kicad-developers
>         <https://launchpad.net/~kicad-developers>
>         > More help   : https://help.launchpad.net/ListHelp
>         <https://help.launchpad.net/ListHelp>
>         >
> 
> 
> 



Follow ups

References