[Insight-developers] Rounding and itkMath.h (Class vs Namespace)

[Bradley Lowekamp]

Hi Tom,

I have a couple questions/comments too:

1) I had to change these statements to the following:

inline vxl_int_32 RoundHalfIntegerToEven_32(float  x) { return  
_mm_cvtss_si32(_mm_set_ss(x)); }
inline vxl_int_64 RoundHalfIntegerToEven_64(float  x) { return  
_mm_cvtss_si64(_mm_set_ss(x)); }

There was a compiler error in regards to mixed up float and double SSE  
vector types, I took a guess that this was the methods intended.

2) And you double check that the following is correct:

#if VNL_CONFIG_ENABLE_SSE2_ROUNDING && defined(__SSE2__) && (defined 
(__x86_64) || defined(__x86_64__) || defined(_M_X64)) && (!defined 
(__GCCXML__))
# define USE_SSE2_64IMPL 1
#else
# define USE_SSE2_64IMPL 0
#endif

I am unsure on why using SSE2 would depend building 64-bits. ?

Answers to your comments and questions follow.

Brad

On Oct 22, 2009, at 5:00 AM, Tom Vercauteren wrote:

> Hey Brad,
>
> Thanks a lot for working through the rounding functions and  
> committing the code!
>
> I just have one question and one comment both referring to itkMath.h.
>
> 1) The question. For revision 1.3, the commit message is
> "COMP: work around for vs6 not correctly supporting function  
> overloading"
> and your patch was to replace
> #ifndef ITK_LEGACY_REMOVE
> by
> #if !defined(ITK_LEGACY_REMOVE) && !(defined(_MSC_VER) && (_MSC_VER  
> <= 1300))
>
> I am not sure I really understand you here. Is this to force vs6 into
> using the new template methods to try and see if we actually need a
> dummy argument (cf.
> http://www.itk.org/mailman/private/insight-developers/2009-August/013231.html
> ) for this compiler? By the way is seems to be the case:
> http://www.cdash.org/CDash/viewBuildError.php?buildid=454305

I am interpreting this error message that vsc6 can not have the  
following type of function overloading:

int foo( int i);
template< typename T > int foo( T i );

Hence I don't think the new and the legacy functions can co-exists on  
vs6. The preprocessor directive is an attempt to remove the legacy  
methods for just vs6. As I don't have access to vs6 and there are only  
a couple of vs6 builds each day, it has been a rather slow experiment  
tying to figure this one out.

The dummy argument doesn't really seem needed because the templeted  
methods is compiling fine in the second half of the itkMathTest.

>
>
> 2) The comment. In the beginning of itkMath.h, you wrote the  
> following comment:
> // note: including Macro.h here will need some thought, which comes  
> first?
>
> To me, it would make sense to include itkMacro.h in itkMath.h because
> itkMath.h may need some macros. Concept checking is one such example.
> However this will break strict backward compatibility because, one
> will have to include itkMath.h in place were rounding fucntions from
> itkMacro.h were already used. I don't think it's such a big deal
> though.
>
> One alternative to avoid this would be to let the non-template
> rounding functions in itkMacro.h and surround then with
> #ifndef ITK_LEGACY_REMOVE
> #endif
> Same thing for the
> #include "itkMath.h"
> in itkMacro.h
>
> Note that I wouldn't vote for this option. Breaking strict backward
> compatibility in the sense that we only require people to add an
> include seems fine to me especially since we already move from
> non-template round to template round.

I think the easiest thing to do will be to include the following at  
the END of itkMacro.h like the following:

#ifndef ITK_LEGACY_REMOVE
#include "itkMath.h"
#endif

This should allow itk Math to use Macro.h, and there be legacy support  
too. I don't see a downside here. Just a little ugly place for the  
include.

========================================================
Bradley Lowekamp
Lockheed Martin Contractor for
Office of High Performance Computing and Communications
National Library of Medicine
blowekamp at mail.nih.gov

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20091022/26b7630b/attachment.htm>