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