kicad-developers team mailing list archive

[Chris Pavlina <pavlina.chris@xxxxxxxxx>]

I'm just going to rename it. I think the optimization was probably unnecessary,
but if it works fine, the only thing it hurts is maintainability, which can be
fixed somewhat by naming it something more obvious. No need to risk introducing
bugs unnecessarily.

Of course personally I'd like to see the parallel type system in EDA_ITEM
disappear entirely, as duplication of information is always error-prone... but
no need to remove this outside of an attempt to purge that. ;)

On Wed, Apr 13, 2016 at 04:21:56PM -0400, Wayne Stambaugh wrote:
> Please proceed with caution.  I don't have any issue with renaming it to
> make the intent clear but thorough real world testing should be done
> before changing over to dynamic_cast<>.
> 
> On 4/13/2016 12:42 PM, Tomasz Wlostowski wrote:
> > On 13.04.2016 18:38, Chris Pavlina wrote:
> >> I would argue - quite strongly! - that it doesn't matter if it's faster in an
> >> isolated test if it isn't used enough to actually affect the overall code
> >> speed. Was this ever profiled in situ? This sort of thing just causes headaches
> >> when people misunderstand the usage and subtleties of the more limited
> >> "optimized" replacement. I find it hard to believe that we call dynamic_cast
> >> enough for it to be a performance issue.
> > 
> > Perhaps you're right, I'm not going to argue here.
> > 
> >>
> >> Also - *please* let me rename it. dynamic_cast vs dyn_cast is almost the
> >> _textboox_ example of poor naming. It's absolutely 100% non-obvious to someone
> >> reading code using the latter _why_ it was chosen, what its advantage is over
> >> the normal one, and whether it has any subtle issues that can cause bugs. At
> >> least it should be called something like dynamic_cast_fast so there's
> >> justification for its use in the actual code using it.
> > 
> > Go on with the renaming.
> > 
> > Tom
> > 
> >>
> >> On Wed, Apr 13, 2016 at 06:34:28PM +0200, Tomasz Wlostowski wrote:
> >>> On 13.04.2016 18:19, Simon Richter wrote:
> >>>> Hi,
> >>>>
> >>>> On 13.04.2016 18:13, Chris Pavlina wrote:
> >>>>
> >>>>> What is the purpose of dyn_cast<> in include/core/typeinfo.h? Why don't we just
> >>>>> use dynamic_cast<>? And can we either replace the former with the latter, or
> >>>>> add a comment to the former explaining its purpose?
> >>>>
> >>>> It uses the parallel type system in EDA_ITEM rather than RTTI, so it
> >>>> works if RTTI is broken, e.g. when compiling with gcc 2.95.
> >>>>
> >>>
> >>> I wrote it inspired with Clang/LLVM design which uses a very similar
> >>> pattern. Sorry Simon, I didn't consider compatibility with gcc 2.95
> >>> would be of an advantage. My reasons were:
> >>>
> >>> 1) Much faster (code in the attachment):
> >>> - dynamic_cast<> : 9090437 usecs
> >>> - dyn_cast<> : 1832433 usecs (5x improvement)
> >>>
> >>> 2) Lightweight & compatible with existing Kicad type system.
> >>>
> >>> Cheers,
> >>> Tom
> >>
> > 
> > 
> > _______________________________________________
> > Mailing list: https://launchpad.net/~kicad-developers
> > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> > Unsubscribe : https://launchpad.net/~kicad-developers
> > More help   : https://help.launchpad.net/ListHelp
> > 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp