MantisBT - ITK
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0007006ITKpublic2008-05-13 10:212010-11-22 17:28
ReporterGert Wollny 
Assigned Tokentwilliams 
PrioritynormalSeveritytweakReproducibilityN/A
StatusclosedResolutionfixed 
PlatformOSOS Version
Product VersionITK-4-A0 
Target VersionFixed in Version 
Resolution Date2010-11-22
Sprint
Sprint Statuscompleted
Summary0007006: typeid should not be used for run-time decisions
DescriptionITK uses typeid in many places to do run-time decisions. Some of these cases can easily be replaced by compile time decisions, specifically by partial template specialization. The enclosed patch replaces typeid constructs in
 - itkFFTRealToComplexConjugateImageFilter.txx
 - itkFFTComplexConjugateToRealImageFilter.txx
 - itkCannySegmentationLevelSetFunction.txx
The respective tests still pass after patch after patch application.
Steps To Reproduce
Additional InformationStroustrup "The C++ Programming Language" Section 15.4.5
Sutter and Alexandrescu "C++ Coding Standards", Item 64, 90
Vandevoorde and Josuttis "C++ Templates", chapter 3.4, 12.4
TagsNo tags attached.
Relationships
Attached Filesdiff InsightToolkit-3.6.0-Code-Algorithms-remove-typeid.diff (6,391) 2008-05-13 10:21
https://public.kitware.com/Bug/file/1459/InsightToolkit-3.6.0-Code-Algorithms-remove-typeid.diff
Issue History
Date ModifiedUsernameFieldChange
2008-05-13 10:21Gert WollnyNew Issue
2008-05-13 10:21Gert WollnyFile Added: InsightToolkit-3.6.0-Code-Algorithms-remove-typeid.diff
2008-05-13 12:01Luis IbanezNote Added: 0011862
2008-05-13 12:02Luis IbanezNote Added: 0011863
2008-05-13 13:35Gert WollnyNote Added: 0011869
2008-05-13 15:40Luis IbanezNote Added: 0011873
2009-07-26 11:04Bill LorensenStatusnew => assigned
2009-07-26 11:04Bill LorensenAssigned To => Bill Lorensen
2009-07-26 11:06Bill LorensenNote Added: 0016971
2009-07-26 11:06Bill LorensenStatusassigned => feedback
2009-07-27 04:14Gert WollnyNote Added: 0016975
2009-07-30 08:03Bill LorensenNote Added: 0017019
2009-07-30 08:04Bill LorensenStatusfeedback => acknowledged
2010-11-04 00:42Bill LorensenNote Added: 0022826
2010-11-04 00:44Bill LorensenNote Added: 0022827
2010-11-04 00:46Bill LorensenSprint Status => backlog
2010-11-04 00:46Bill LorensenResolutionopen => suspended
2010-11-04 15:17Hans JohnsonStatusacknowledged => assigned
2010-11-04 15:17Hans JohnsonAssigned ToBill Lorensen => kentwilliams
2010-11-04 15:17Hans JohnsonNote Added: 0022849
2010-11-04 15:17Hans JohnsonResolutionsuspended => reopened
2010-11-05 12:58kentwilliamsNote Added: 0022900
2010-11-05 13:19kentwilliamsResolution Date => 2010-11-05
2010-11-05 13:19kentwilliamsSprint Statusbacklog => active
2010-11-05 13:19kentwilliamsStatusassigned => resolved
2010-11-05 13:19kentwilliamsProduct Version => ITK-4-A0
2010-11-05 14:04Gert WollnyNote Added: 0022911
2010-11-05 14:04Gert WollnyStatusresolved => feedback
2010-11-22 17:28kentwilliamsResolution Date2010-11-05 => 2010-11-22
2010-11-22 17:28kentwilliamsSprint Statusactive => completed
2010-11-22 17:28kentwilliamsNote Added: 0023499
2010-11-22 17:28kentwilliamsStatusfeedback => closed
2010-11-22 17:28kentwilliamsResolutionreopened => fixed

Notes
(0011862)
Luis Ibanez   
2008-05-13 12:01   
Unfortunately we can not use partial specialization because it is not supported in Visual Studio 6.0 and we still support this compiler.
(0011863)
Luis Ibanez   
2008-05-13 12:02   
How about replacing instances of typeid with using "dynamic_cast" ?
(0011869)
Gert Wollny   
2008-05-13 13:35   
* To cite Strounstrup: "Using dynamic_cast rather than typeid would improve code quality only marginally" - it would still be a run-time decision - and in the FFTW application cases it would not be possible because the typeid is applied to identify float and double pixel types.

* Still VC 6.0 support, seems you are better than MS. VC6 is now 10 years old, maybe one should ... hmm ... move on? I mean, one can download VC++ Express 2008 for free (haven't tried it, though).
(0011873)
Luis Ibanez   
2008-05-13 15:40   
We have requested feedback from the users list:

http://www.itk.org/pipermail/insight-users/2008-May/025838.html [^]

regarding how many users are still relying on Visual Studio 6.0.
(0016971)
Bill Lorensen   
2009-07-26 11:06   
I agree that partial specialization is a better method than run-time decisions based on typeid. However, not all of the supported ITK compilers support partial specialization.
(0016975)
Gert Wollny   
2009-07-27 04:14   
Maybe that's something to be left for version 4.0: to redefine the set of supported compilers, make sure partial specialization is part of the required feature set, and then fix these kind f issues.
(0017019)
Bill Lorensen   
2009-07-30 08:03   
We will leave this issue open as a reminder of the limitations of some of our current compilers.
(0022826)
Bill Lorensen   
2010-11-04 00:42   
Now that we have dropped VS6,7 and Borland, we can address this issue.
(0022827)
Bill Lorensen   
2010-11-04 00:44   
CannySegmentationLevelSetFunction has been fixed. See this gerrit topic:
http://review.source.kitware.com/#change,240 [^]
(0022849)
Hans Johnson   
2010-11-04 15:17   
The patch can help get rid of many typeid constructs, and can now be applied now that partial specialization is allowed.
(0022900)
kentwilliams   
2010-11-05 12:58   
I'm readying a push to gerrit that more or less comprehensively addresses this idea of removing typeids.

Notes:

1. Gaetan has a separate rewrite of Morphology-related classes that removes a bunch of instances.
2. Many classes use typeid to get a type name string for debugging -- not fixed.
3. Some classes have long lists of typeids, that -- as in the ImageIO cases -- aren't easily replaced
4. typeid -> type name string is another common use.
5. ITK/Code/Review/itkFFTComplexToComplexImageFilter has some stylistic problems, notably no implementation unencumbered by FFTW. Fixing this would require more work than I can put into it in a reasonable timeframe.
(0022911)
Gert Wollny   
2010-11-05 14:04   
some remarks:

2 - Using typeid to get a type name for debugging is not the issue, making run-time decisions based on typeid is, since type based decisions can be resolved at compile time and errors will be reported early. Also, run-time decisions add a performance penalty that may be of consequence.

3 - at least what i've seen in ImageIO looks like the perfect opportunity to use a pixel type trait class. It also makes it easier to add another type, because one only needs to define a new specialization of the trait together with the new type and one doesn't have to touch the long list of ifs.
Disadvantage: for this to function the code needs to be templated.

4 - same resolution as 3.
(0023499)
kentwilliams   
2010-11-22 17:28   
One fix has been pushed already, and I've added a new topic to address the remaining issues I could discover:
http://review.source.kitware.com/#change,424 [^]

There are typeids left but they're either a) using a typeid for printing debug information, or b) a run-time type check that can't be replaced with Template Metaprogramming.