kicad-developers team mailing list archive

[Jeff Young <jeff@xxxxxxxxx>]

> On 4 Jul 2020, at 00:31, Reece R. Pollack <reece@xxxxxxx> wrote:
> 
> On 7/3/20 5:42 PM, Jeff Young wrote:
>> Hi Reece,
>> 
>>> On 3 Jul 2020, at 21:32, Reece R. Pollack <reece@xxxxxxx <mailto:reece@xxxxxxx>> wrote:
>>> 
>>> Noting that the PCB_BASE_FRAME class is derived from the EDA_DRAW_FRAME class, is it acceptable to assume that the EDA_DRAW_FRAME pointer parameters passed to functions in Pcbnew classes are actually pointers to a PCB_BASE_FRAME?
>> 
>> Yes, but it would be considered bad practice.
> 
> I would argue that it could easily be defined that code in the pcbnew directory is dealing with a PCB_BASE_FRAME rather than an EDA_DRAW_FRAME. Common code should not make that assumption, of course, and would not use anything beyond the base class.

The problem is not when the code is written, but rather years later when someone changes one of the assumptions you were depending on and doesn’t even remember that your code exists.  dynamic_cast will then protect you, while a C-style or static_cast is likely going to do something ugly.

> 
>>> 
>>> Specifically:
>>> The UNIT_BINDER class constructor
>>> The classes implementing the GetMsgPanelInfo function
>>> The classes implementing the GetSelectMenuText function
>>> The reason I'm asking is that to implement origin transforms these functions need access to the user's display option that chooses the display origin. This needs access to a function defined in the PCB_BASE_FRAME class. I can make that a common function defined in EDU_DRAW_FRAME and overridden in PCB_BASE_FRAME, or the code needs to know that parameter is really a pointer to a PCB_BASE_FRAME object.
>>> 
>> Ask yourself this: is there anything PCB-specific about a settable origin?  Is there any reason a settable origin wouldn’t also be useful, say, a schematic?  If the answer is “no”, then you should push the origin (along with its getters and setters) down in to EDA_DRAW_FRAME.
> 
> My submission last year was intended to allow any or all of the KiCad subsystems (eeschema, pcbnew, gerbview, etc.) make use of origin transforms. Gerbview could benefit, because if you set the place and drill origin to the lower left of you design when you generate Gerbers, Gerbview displays the page border below the displayed board. I'd thought about how to do this in eeschema, considering an origin at any of the four corners or at the aux origin (which is defined in eeschema but is not settable). But I did a full implementation on pcbnew because that was what I felt really needed it.That  implementation added a parameter to the functions I mentioned above, which resulted in the eeschema functions also having that parameter even though they didn't use it. 
> 
> When Wayne sent out his last call for comments before he merged the changes, Seth complained that since I didn't implement support in eeschema then these changes should not affect any code in the eeschema directory. His suggestion was to use function overloading, but that would result in having two interfaces visible in the both eeschema and pcbnew, one that worked in that context and one that didn't. You wouldn't know that the code called the wrong function until you noticed broken behavior. I don't consider that an acceptable situation just to avoid a trivial parameter list change.
> 
> My approach this year is that code in the pcbnew directory knows it's dealing with a PCB_BASE_FRAME because it's PCB-specific code in the first place. Then it's easy to access the PCB-specific interfaces and data. Rather than changing the UNIT_BINDER class I've implemented a PCB_UNIT_BINDER class derived from UNIT_BINDER. It knows that it's getting a PCB_BASE_FRAME rather than an EDA_DRAW_FRAME, and passes that to the base class constructor. It's more tricky with the functions that are called indirectly through the EDA_DRAW_FRAME, such as GetMsgPanelInfo and GetSelectMenuText. The implementations are specific to pcbnew and live in the pcbnew directory.
> 
> I can do it with dynamic casts, but I'm an embedded systems guy and I hate to waste CPU cycles checking things that are defined to be true. And dynamic_cast is slow: my tests of the Icarus Verilog simulator appear to show VVP spends about 20% of its CPU cycles just resolving dynamic casts. 
> 
>> If this really is PCB-specific, then you can either push just the origin getters/setters down into EDA_DRAW_FRAME and override them in PCB_BASE_FRAME, or do a dynamic_cast to PCB_BASE_FRAME and if non-null call the getters/setters.
> 
> I believe there will need to be subsystem-specific implementations of portions of the origin transform code, based on what display options we offer to the user. However, I think a lot of the interfaces can be common. I'm willing to do either a PCB-only version or one that has support for pcbnew and latent support for the other subsystems. What I'm not willing to do is invest a lot of effort, only for someone to veto it at the last minute over a stylistic difference of opinion again. We, meaning myself and the core developers collectively, need to come to a consensus on how this should be implemented.

Bummer.  I, for one, think last year’s approach was better.  But I’m not going to veto either one….

Cheers,
Jeff.

> 
> -Reece