kicad-developers team mailing list archive
-
kicad-developers team
-
Mailing list archive
-
Message #28809
Re: [PATCH] Refactor LAYER_ID to be the one and only layer definition
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>>
> > >
> >
> >
> >
>
>
Follow ups
References
-
[PATCH] Refactor LAYER_ID to be the one and only layer definition
From: Jon Evans, 2017-03-13
-
Re: [PATCH] Refactor LAYER_ID to be the one and only layer definition
From: Wayne Stambaugh, 2017-03-13
-
Re: [PATCH] Refactor LAYER_ID to be the one and only layer definition
From: Jon Evans, 2017-03-13
-
Re: [PATCH] Refactor LAYER_ID to be the one and only layer definition
From: Maciej Suminski, 2017-03-13
-
Re: [PATCH] Refactor LAYER_ID to be the one and only layer definition
From: Jon Evans, 2017-03-13
-
Re: [PATCH] Refactor LAYER_ID to be the one and only layer definition
From: Jon Evans, 2017-03-14
-
Re: [PATCH] Refactor LAYER_ID to be the one and only layer definition
From: John Beard, 2017-03-14
-
Re: [PATCH] Refactor LAYER_ID to be the one and only layer definition
From: Jon Evans, 2017-03-14
-
Re: [PATCH] Refactor LAYER_ID to be the one and only layer definition
From: Jon Evans, 2017-03-14
-
Re: [PATCH] Refactor LAYER_ID to be the one and only layer definition
From: Wayne Stambaugh, 2017-03-14
-
Re: [PATCH] Refactor LAYER_ID to be the one and only layer definition
From: Jon Evans, 2017-03-16
