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