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.be...@gmail.com> 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 <j...@craftyjon.com> 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 <j...@craftyjon.com> 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.sumin...@cern.ch> > >> 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 < > stambau...@gmail.com> > >>> > 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@lists.launchpad.net > >>> >>> Unsubscribe : https://launchpad.net/~kicad-developers > >>> >>> More help : https://help.launchpad.net/ListHelp > >>> >>> > >>> >> > >>> >> _______________________________________________ > >>> >> Mailing list: https://launchpad.net/~kicad-developers > >>> >> Post to : kicad-developers@lists.launchpad.net > >>> >> Unsubscribe : https://launchpad.net/~kicad-developers > >>> >> More help : https://help.launchpad.net/ListHelp > >>> >> > >>> > > >>> > > >>> > > >>> > _______________________________________________ > >>> > Mailing list: https://launchpad.net/~kicad-developers > >>> > Post to : kicad-developers@lists.launchpad.net > >>> > Unsubscribe : https://launchpad.net/~kicad-developers > >>> > More help : https://help.launchpad.net/ListHelp > >>> > > >>> > >>> _______________________________________________ > >>> Mailing list: https://launchpad.net/~kicad-developers > >>> Post to : kicad-developers@lists.launchpad.net > >>> Unsubscribe : https://launchpad.net/~kicad-developers > >>> More help : https://help.launchpad.net/ListHelp > >> > >> > > > > > > _______________________________________________ > > Mailing list: https://launchpad.net/~kicad-developers > > Post to : kicad-developers@lists.launchpad.net > > Unsubscribe : https://launchpad.net/~kicad-developers > > More help : https://help.launchpad.net/ListHelp > > >
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp