[Insight-developers] Rounding and itkMath.h (Class vs Namespace)
Tom Vercauteren
tom.vercauteren at m4x.org
Thu Oct 22 05:00:33 EDT 2009
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
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.
Regards,
Tom
On Fri, Oct 9, 2009 at 09:54, Tom Vercauteren <tom.vercauteren at m4x.org> wrote:
> Hi Brad,
>
> On Thu, Oct 8, 2009 at 16:22, Bradley Lowekamp <blowekamp at mail.nih.gov> wrote:
>>
>> I have not devoted as much time to Math rounding method as I was hopping. I
>> plan to do the first check in of this on Monday.
>> One change I hope to make is to separate the implementation from the header
>> file. As the Detail namespace declares and defines functions for inlining
>> purposes, I don't think that making a "txx" file would be appropriate. I
>> propose an itkMathDetail.h file for the "private" internal functions, which
>> don't need to be in doxygen.
>
> Sounds good. I like the idea of hiding the gory details, especially
> with all the ifdef involved. If it still is too messy, another option
> would be to have one "MathDetail" file per architecture:
> * itkMathDetailVanilla.h
> * itkMathDetailSSE2.h
> * itkMathDetailGCCX86ASM.h
> * itkMathDetailMSVCX86ASM.h
> * etc.
>
> In any case, it's nice not to show this in doxygen.
>
>> Second, I know we designed the TReturn to expect integer types, but as it
>> currently is it can be instantiated with real types as well. Also we must
>> consider the behavior of unsigned integer return types as well. This gives
>> us two options that I see, make it so it only compiles with integer types,
>> or add support for real types.
>
> Make it so it only compiles with integer types seems sufficient to me.
> Bu if you feel we need to support real types, go ahead.
>
> In our code we supported real types and unsigned types using boost mpl
> (not sure hwo to do this using ITK only). The following code should be
> sufficient to understand, just replace the iRound by you preferred
> rounding function.
>
> // Private function: rounding necessary
> template<typename T, typename U>
> inline U roundIfFloatingToIntegral_(T x, typename
> boost::enable_if<boost::mpl::and_<
> const boost::is_floating_point<T>,
> boost::is_integral<U> > >::type * = 0)
> {
> return iRound(x);
> }
>
> // Private function: no rounding since we convert to floating point
> template<typename T, typename U>
> inline U roundIfFloatingToIntegral_(T x, typename
> boost::disable_if<boost::mpl::and_<
> const boost::is_floating_point<T>,
> boost::is_integral<U> > >::type * = 0)
> {
> return static_cast<U> (x);
> }
>
> // convert T type to U type using round if U is integer (no special
> care taken for unsigned)
> template<typename T, typename U>
> inline U roundIfFloatingToIntegral(T x)
> {
> return roundIfFloatingToIntegral_<T,U> (x);
> }
>
> // Private function: use smart T to U conversion and thresholding
> above zero because U is unsigned
> template<typename T, typename U>
> inline U roundAndBoundIfFloatingToIntegral_(T x, typename
> boost::enable_if<boost::mpl::and_<
>
> boost::mpl::not_<boost::is_unsigned<T> >,
> const
> boost::is_unsigned<U> > >::type * = 0)
> {
> if(x > static_cast<T>(0)) return roundIfFloatingToIntegral<T,U>(x);
> else return static_cast<U>(0);
> }
>
> // Private function: use smart T to U conversion, no need to threshold
> above zero because U is signed
> template<typename T, typename U>
> inline U roundAndBoundIfFloatingToIntegral_(T x, typename
> boost::disable_if<boost::mpl::and_<
>
> boost::mpl::not_<boost::is_unsigned<T> >,
> const
> boost::is_unsigned<U> > >::type * = 0)
> {
> return roundIfFloatingToIntegral<T,U>(x);
> }
>
> // convert T type to U type using round if U is integer and
> thresholding above zero if U is unsigned
> template<typename T, typename U>
> inline U roundAndBoundIfFloatingToIntegral(T x)
> {
> return roundAndBoundIfFloatingToIntegral_<T,U> (x);
> }
>
>
>
>> Third, we need more documentation. Does the
>> itkTemplateFloatingToIntegerMacro work well with the Doxygen, so that
>> Doxygen will make function documentation? I think is should since the
>> SetGetMacros work. The documentation for the rounding methods should include
>> a description expected usage, limitations and suggestions or references to
>> where they should be used in ITK.
>> Brad
>> On Oct 1, 2009, at 10:37 AM, Tom Vercauteren wrote:
>>
>> Hey Brad,
>>
>> Moving forward on this would be very nice!
>>
>> I think the first step to solve before implementing improved template
>> rounding functions is to get fixed-width (32 and 64 bit) integers. As
>> I reported on the bug tracker
>> http://www.itk.org/Bug/view.php?id=9426
>> I believe that using pstdint.h
>> http://www.azillionmonkeys.com/qed/pstdint.h
>> should be the easiest portable way to go for that.
>>
>> Regarding the dummy argument. It's right that I have not used it in my
>> itk-templatedmathround-2009-07-30.patch patch:
>> http://www.itk.org/Bug/file_download.php?file_id=2390&type=bug
>> The reason is that the dummy argument should only be necessary for
>> MSVC 6 but since I have no access to this compiler, this is something
>> I cannot be sure of. Trying first without the dummy argument and
>> adding it only if the MSVC 6 dashboard build fails seems a reasonable
>> plan.
>>
>> My two cents,
>> Tom
>>
>> P.S.: I don't have much time to code on this right now but I'll try to
>> be responsive to emails.
>>
>> On Thu, Oct 1, 2009 at 16:14, Bradley Lowekamp <blowekamp at mail.nih.gov>
>> wrote:
>>
>> Hello,
>>
>> I would like to begin to determine the best way to implement the improved
>>
>> rounding methods across all the compilers we currently support. The plan is
>>
>> to create a itkMath.h file, and not include it int Macro.h until we have
>>
>> determined the best implementation. Then create a standalone test ( not part
>>
>> of a CTest driver ) to determine what is going to work best. This should
>>
>> only create one failing test/build until this is all figured out.
>>
>> Tom,
>>
>> The version in mantis is recent and doesn't include the dummy argument yet?
>>
>> The initial implementation we should try is without this dummy argument in a
>>
>> namespace.
>>
>> Brad
>>
>> On Aug 11, 2009, at 8:01 AM, Tom Vercauteren wrote:
>>
>> Hi Brad,
>>
>> From a totally subjective point of view, I prefer the namespace
>>
>> approach. The only issue I still have is about the backward
>>
>> compatibility policy. The rounding functions need some helper
>>
>> functions. It would be better if those helper functions were not
>>
>> subject to any backward compatibility requirement. Here are three
>>
>> possibilities I can think of:
>>
>>
>> 1) All namespace approach
>>
>> ---------------------------------------
>>
>> Advantage: seems clearer to me
>>
>> Drawback: No control on the access to th helper functions in Math::Detail
>>
>> namespace Math
>>
>> {
>>
>> const double p = 3.14;
>>
>> namespace Detail
>>
>> {
>>
>> // Things here are excluded from the backward-compatibility policy
>>
>> template <typename TReturn, typename TInput>
>>
>> inline TReturn RoundHelper(TInput x, TReturn * = 0) { [snip] }
>>
>> }
>>
>> // The second argument is fro msvc 6 compatibility
>>
>> template <typename TReturn, typename TInput>
>>
>> inline TReturn Round(TInput x, TReturn * = 0)
>>
>> {
>>
>> return Detail::RoundHelper<TReturn>(x);
>>
>> }
>>
>> }
>>
>>
>>
>> 2) All class approach
>>
>> --------------------------------
>>
>> Advantage: Helper functions not accessible to the users
>>
>> Drawback: Somehow confusing for me to have a class with only static
>>
>> members and functions (Also potential compiler optimization loss??)
>>
>> class Math
>>
>> {
>>
>> public:
>>
>> static const double pi;
>>
>> template <typename TReturn, typename TInput>
>>
>> static inline TReturn Round(TInput x, TReturn * = 0)
>>
>> {
>>
>> return RoundHelper<TReturn>(x);
>>
>> }
>>
>> private:
>>
>> template <typename TReturn, typename TInput>
>>
>> static inline TReturn RoundHelper(TInput x, TReturn * = 0) { [snip] }
>>
>> };
>>
>> const double Math::pi = 3.14;
>>
>>
>>
>> 3) Mixed namespace-class approach
>>
>> -----------------------------------------------------
>>
>> Advantage: Helper functions not accessible to the users, constants are
>>
>> defined next to their declaration
>>
>> Drawback: More complex code due to template friend functions
>>
>> namespace Math
>>
>> {
>>
>> const double p = 3.14;
>>
>> // Forward declarations
>>
>> class MathDetail;
>>
>> template<typename TReturn, typename TInput>
>>
>> TReturn Round(TInput, TReturn *);
>>
>> class MathDetail
>>
>> {
>>
>> public:
>>
>> template<typename TReturn, typename TInput>
>>
>> friend inline TReturn Round(TInput x, TReturn * = 0);
>>
>> private:
>>
>> template <typename TReturn, typename TInput>
>>
>> static inline TReturn RoundHelper(TInput x, TReturn * = 0) { [snip] }
>>
>> }
>>
>> template <typename TReturn, typename TInput>
>>
>> inline TReturn Round(TInput x, TReturn * = 0)
>>
>> {
>>
>> return MathDetail::RoundHelper<TReturn>(x);
>>
>> }
>>
>> }
>>
>>
>> It seems to me we should wait until after the release to implement any
>>
>> of these option. What do you think?
>>
>> Cheers,
>>
>> Tom
>>
>>
>> On Sat, Aug 8, 2009 at 16:18, Bradley Lowekamp<blowekamp at mail.nih.gov>
>>
>> wrote:
>>
>> Currently we have two things we would like to put into itk::Math, weather it
>>
>> be a namespace or a class. The first is the rounding methods and the second
>>
>> is the numeric constants. For the numeric constants consider these two
>>
>> different possibilities:
>>
>>
>> math.h:
>>
>> class Math
>>
>> {
>>
>> static const double p;
>>
>> }
>>
>> math.cxx:
>>
>> const double p = 3.14;
>>
>> Where as with a namespace, we don't need the static qualifier and I believe
>>
>> the following will not have the linking issues, and is valid c++:
>>
>> math.h:
>>
>> namespace Math {
>>
>> const double p = 3.14;
>>
>> };
>>
>>
>> With the namespace implementation we can always get optimizations, but there
>>
>> will be multiple addresses in different compilation files. I don't believe
>>
>> that that latter is really a negative though.
>>
>>
>> Brad
>>
>>
>> ========================================================
>>
>> Bradley Lowekamp
>>
>> Lockheed Martin Contractor for
>>
>> Office of High Performance Computing and Communications
>>
>> National Library of Medicine
>>
>> blowekamp at mail.nih.gov
>>
>>
>>
>> ========================================================
>>
>> Bradley Lowekamp
>>
>> Lockheed Martin Contractor for
>>
>> Office of High Performance Computing and Communications
>>
>> National Library of Medicine
>>
>> blowekamp at mail.nih.gov
>>
>>
>
More information about the Insight-developers
mailing list