Hi Wayne, Any feedback on this?
Thanks, Jon On Wed, Mar 22, 2017 at 6:15 AM, Maciej Sumiński <maciej.sumin...@cern.ch> wrote: > I have briefly tested the patch, no issues found. I have no objections > to the changes, but I will leave the final decision to Wayne. > > Regards, > Orson > > On 03/22/2017 03:51 AM, Jon Evans wrote: > > Hi Wayne, new patch attached. > > > > Thanks, > > Jon > > > > On Tue, Mar 21, 2017 at 9:28 AM, Wayne Stambaugh <stambau...@gmail.com> > > wrote: > > > >> Jon, > >> > >> I just attempted to apply your patch and it no longer applies cleanly. > >> Please rebase it when you get a chance. > >> > >> Thanks, > >> > >> Wayne > >> > >> On 3/16/2017 10:14 AM, Jon Evans wrote: > >>> 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 > >>> <mailto: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> > >>> > <mailto: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> > >>> > <mailto: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> > >>> > > >>> <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> > >>> > <mailto: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> > >>> > <mailto: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 > > > >>> <mailto: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> > >>> > <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> > >>> <mailto: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> > >>> > <https://launchpad.net/~kicad-developers > >>> <https://launchpad.net/~kicad-developers>> > >>> > >>> >>> Post to : kicad-developers@lists. > launchpad.net > >>> <mailto:kicad-developers@lists.launchpad.net> > >>> > <mailto:kicad-developers@lists.launchpad.net > >>> <mailto:kicad-developers@lists.launchpad.net>> > >>> > >>> >>> Unsubscribe : https://launchpad.net/~kicad- > >> developers > >>> <https://launchpad.net/~kicad-developers> > >>> > <https://launchpad.net/~kicad-developers > >>> <https://launchpad.net/~kicad-developers>> > >>> > >>> >>> More help : https://help.launchpad.net/ > ListHelp > >>> <https://help.launchpad.net/ListHelp> > >>> > <https://help.launchpad.net/ListHelp > >>> <https://help.launchpad.net/ListHelp>> > >>> > >>> >>> > >>> > >>> >> > >>> > >>> >> _______________________________________________ > >>> > >>> >> Mailing list: https://launchpad.net/~kicad- > >> developers > >>> <https://launchpad.net/~kicad-developers> > >>> > <https://launchpad.net/~kicad-developers > >>> <https://launchpad.net/~kicad-developers>> > >>> > >>> >> Post to : kicad-developers@lists. > launchpad.net > >>> <mailto:kicad-developers@lists.launchpad.net> > >>> > <mailto:kicad-developers@lists.launchpad.net > >>> <mailto:kicad-developers@lists.launchpad.net>> > >>> > >>> >> Unsubscribe : https://launchpad.net/~kicad- > >> developers > >>> <https://launchpad.net/~kicad-developers> > >>> > <https://launchpad.net/~kicad-developers > >>> <https://launchpad.net/~kicad-developers>> > >>> > >>> >> More help : https://help.launchpad.net/ListHelp > >>> <https://help.launchpad.net/ListHelp> > >>> > <https://help.launchpad.net/ListHelp > >>> <https://help.launchpad.net/ListHelp>> > >>> > >>> >> > >>> > >>> > > >>> > >>> > > >>> > >>> > > >>> > >>> > _______________________________________________ > >>> > >>> > Mailing list: https://launchpad.net/~kicad- > >> developers > >>> <https://launchpad.net/~kicad-developers> > >>> > <https://launchpad.net/~kicad-developers > >>> <https://launchpad.net/~kicad-developers>> > >>> > >>> > Post to : kicad-developers@lists.launchpad.net > >>> <mailto:kicad-developers@lists.launchpad.net> > >>> > <mailto:kicad-developers@lists.launchpad.net > >>> <mailto:kicad-developers@lists.launchpad.net>> > >>> > >>> > Unsubscribe : https://launchpad.net/~kicad- > >> developers > >>> <https://launchpad.net/~kicad-developers> > >>> > <https://launchpad.net/~kicad-developers > >>> <https://launchpad.net/~kicad-developers>> > >>> > >>> > More help : https://help.launchpad.net/ListHelp > >>> <https://help.launchpad.net/ListHelp> > >>> > <https://help.launchpad.net/ListHelp > >>> <https://help.launchpad.net/ListHelp>> > >>> > >>> > > >>> > >>> > >>> > >>> _______________________________________________ > >>> > >>> Mailing list: https://launchpad.net/~kicad- > developers > >>> <https://launchpad.net/~kicad-developers> > >>> > <https://launchpad.net/~kicad-developers > >>> <https://launchpad.net/~kicad-developers>> > >>> > >>> Post to : kicad-developers@lists.launchpad.net > >>> <mailto:kicad-developers@lists.launchpad.net> > >>> > <mailto:kicad-developers@lists.launchpad.net > >>> <mailto:kicad-developers@lists.launchpad.net>> > >>> > >>> Unsubscribe : https://launchpad.net/~kicad- > developers > >>> <https://launchpad.net/~kicad-developers> > >>> > <https://launchpad.net/~kicad-developers > >>> <https://launchpad.net/~kicad-developers>> > >>> > >>> More help : https://help.launchpad.net/ListHelp > >>> <https://help.launchpad.net/ListHelp> > >>> > <https://help.launchpad.net/ListHelp > >>> <https://help.launchpad.net/ListHelp>> > >>> > >> > >>> > >> > >>> > > > >>> > > > >>> > > _______________________________________________ > >>> > > Mailing list: https://launchpad.net/~kicad-developers > >>> <https://launchpad.net/~kicad-developers> > >>> > <https://launchpad.net/~kicad-developers > >>> <https://launchpad.net/~kicad-developers>> > >>> > > Post to : kicad-developers@lists.launchpad.net > >>> <mailto:kicad-developers@lists.launchpad.net> > >>> > <mailto:kicad-developers@lists.launchpad.net > >>> <mailto:kicad-developers@lists.launchpad.net>> > >>> > > Unsubscribe : https://launchpad.net/~kicad-developers > >>> <https://launchpad.net/~kicad-developers> > >>> > <https://launchpad.net/~kicad-developers > >>> <https://launchpad.net/~kicad-developers>> > >>> > > More help : https://help.launchpad.net/ListHelp > >>> <https://help.launchpad.net/ListHelp> > >>> > <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 > > > > > > _______________________________________________ > 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
