kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

On 3/14/2017 10:08 AM, Jon Evans 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"

This is a good thing not a bad thing (see below).

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

I'm not opposed to this.

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

I'm not opposed to this.

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

Although I didn't do any testing, I like enum inheritance solution
compared to the one giant enum solution.  It allows you to maintain enum
specific function behavior while still providing compile time safety
(you can't use an enum as a function argument that doesn't handle it).
One of the main problems I see from the giant enum solution is that you
could inadvertently use one of the virtual or schematic layer enums to
call functions that specifically requires a board layer enum.  This
behavior is undefined and would be difficult to debug.  Once of the
primary goals of enums is compile time safety.  Having a giant enum is
not much better than casting enums to ints.  Technically, any function
that takes a LAYER_ID enum should give a valid result for any enum in
the list.  The giant enum solution would require every single function
that takes this enum as an argument to cull out the enums that are not
relevant which is confusing.  I actually think enum inheritance
technique is more readable since the separate enums make it clear that
they are not related except when they are merged together for the color
configuration purposes.  As for enum redefinition using the inheritance
technique, I don't think the union with allow for duplicate argument
names.  It should be fairly simple to test.

Cheers,

Wayne

> 
> 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
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>