kicad-developers team mailing list archive

[Jon Evans <jon@xxxxxxxxxxxxx>]

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