kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

On 06/06/2014 03:22 PM, Lorenzo Marcantonio wrote:
> On Fri, Jun 06, 2014 at 08:30:59PM +0200, Tomasz Wlostowski wrote:
> 
>> Either use only C++ RTTI or a type ID field. Mixing both is ugly.
> 
> Or better yet trust your interfaces:P if you need to know the kind of
> object you have, then there is a design issue, usually. Also type ID
> doesn't handle inheritance easily, which is bad.
> 
>> This means a pretty much complete rewrite of Kicad...
> 
> Of course. That was meant for *new* code.
> 
>> I was reading Clang/LLVM source code and found this dyn_cast/isa pattern
>> very elegant and lightweight. It's a mine of practical OO design ideas.
> 
> I could argue about the elegance of the whole template cruft:P it seems
> that in 'modern' C++ you need to write more boilerplate/magical
> definition than actual 'work' code.
> 
>> - it's much faster than C++ dynamic_cast,
> 
> Same analysis I did.
> 
>> - you don't have to remember the type ID <> class association (e.g.
>> PCB_LINE_T vs class DRAWSEGMENT)
> 
> Have you looked for using typeid? could be an interesting alternative
> (altough I don't seen any obvious advantage over the kicad type tag,
> other than being a standard construct)
> 
>> - can be implemented without disrupting the code
> 
> Other than all these ugly things around :D
> 
>> - saves a line (or three lines) of code on every down-cast:
> 
> Agree with that. Still I found that usually you need to check the type
> before the downcast anyway, so you have to do the if statement... also
> the downcasted pointer has reduced scope which is IMHO a big advantage.
> 
> The common pattern I've found is:
> 
> if (stuff->type is CLASS_T) {
>     CLASS *ptr = static_cast<CLASS*>(stuff);
>     // do things with ptr
> }
> 
> with your solution the pattern would be:
> 
> CLASS *ptr = dyn_cast<CLASS>(stuff); // More or less
> if (ptr) {
>     // do thing with ptr
> }
> 
> When you expand the template the resulting code is pretty much the same:
> 
> CLASS *ptr = (stuff->type is CLASS_T) ? static_cast<CLASS*>(stuff) : NULL;
> if (ptr) {
>     // do thing with ptr
> }
> 
> In the end it's down to individual preference, almost like indentation
> conventions...
> 
> For the other people however I'd add a big comment explaining how it
> works and the limitations (i.e. you need to define the correct ClassOf
> statics and maybe other things). I guess it *could* handle inheritance,
> putting the closure of the derived classes in the parent (so that an
> hypotetical EDA_ITEM::ClassOf would contain almost all of the class
> tags:P)
> 
> I've had bad experiences wrestling with the TRACK/VIA/SEGZONE class
> branch virtuals...
> 


In PAINTER and in a few other places there are dispatches using a switch().  A switch(),
on a good day, can generate much more efficient code than a series of else if()s.

On that good day the switch() code generater will use a jump table with machine code which
could also come from this type of construct:

(*jump_table[switch_enum - switch_lowest_value])()

If gaps in the enum are not overwhelming, then those will get filled with jumps to
"default:".  The main point is that if this is in a drawing loop, then it becomes a
hotspot.  Not having multiple else if tests makes it very lean.  There are no tests, only
one indexed lookup.

We moved PAINTER out of the object hierarchy for good reasons.

Unless you can match the kind of performance that a large switch() provides, I think this
ClassOf is not a good path to entertain.

Nor does it fix anything.


Dick