kicad-developers team mailing list archive

[Jon Evans <jon@xxxxxxxxxxxxx>]

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> 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
>
> On Tue, Mar 14, 2017 at 10:08 PM, Jon Evans <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> 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>
> >> 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
> >>>
> >>> 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>
> >>> > 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
> >>> >>> 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
> >>> >>
> >>> >
> >>> >
> >>> >
> >>> > _______________________________________________
> >>> > 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
> >>
> >>
> >
> >
> > _______________________________________________
> > 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
> >
>