kicad-developers team mailing list archive

[Jon Evans <jon@xxxxxxxxxxxxx>]

Hi Wayne,

Please keep in mind that anywhere there is currently int casting and bounds
checking that deals with colors, will be replaced with type-safe methods in
an upcoming patch from me.  For those places not dealing with colors, I
will see if it can be improved in a future patch.

Thanks,
Jon

On Mar 14, 2017 19:06, "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>
>         >
>
>
>