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 >
signature.asc
Description: OpenPGP digital signature
_______________________________________________ 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
