Re: [eigen] Eigen::FFT and Eigen::Complex in repo

[Benoit Jacob <jacob.benoit.1@xxxxxxxxx>]

2009/10/22 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
>
>
> On Wed, Oct 21, 2009 at 4:15 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx>
> wrote:
>>
>> 2009/10/21 Mark Borgerding <mark@xxxxxxxxxxxxxx>:
>> > Benoit Jacob wrote:
>> >>
>> >> FFT line 48:
>> >>
>> >> template <typename _Scalar,
>> >>         typename _Traits=DEFAULT_FFT_IMPL<_Scalar>
>> >>         >
>> >>
>> >> First, why call it Traits when Impl seems to be what it really is?
>> >
>> > Done.
>> >
>> >
>> >> Also, here you're passing _Scalar twice, and are using a macro. You
>> >> can improve on both fronts in 2 ways:
>> >> * either you let _Scalar be deduced from _Impl
>> >>
>> >
>> > My thinking behind the dual argument was to allow quick and easy
>> > declaration
>> > from the user standpoint.
>> > e.g.
>> >   FFT<float>
>> > where the implementation that gets used is the best available at compile
>> > time. Rather than
>> >   FFT< ei_kissfft_impl<float> >
>> > which seems verbose and inflexible
>> > (also notice the error prone "> >" )
>>
>> ah, good point, so nevermind this idea.
>>
>> >
>> > I agree that the use of preprocessor macros to define the default is
>> > less
>> > than ideal. Perhaps it should not default to the best available, but
>> > rather
>> > to whichever library is requested via preprocessor definitions.
>> >
>> >
>> >> * or you can keep the _Scalar parameter and let _Impl be a template
>> >> template parameter:
>> >>
>> >> template <typename _Scalar, template<typename t> class _Impl>
>> >> class FFT
>> >> {
>> >> ...
>> >> };
>> >>
>> >
>> > I have not done this before. I will give it a try.
>>
>> Yes it now seems like the way to go!
>
> I'm not sure about that, because with that solution you wont be able to
> declare backends with more that one template parameter. Think, e.g., about a
> fixed size backend or any other backend which could take advantage of
> compile time options...

Indeed!

Anyway, have you seen Mark's latest commit? It does

template <typename _Scalar,
         typename _Impl=default_fft_impl<_Scalar> >

in other words he just kept his original approach and got rid of the
macro. I can understand now, how, given the mechanism for default
backends, this is better than what I proposed, as the user only
specifies explicitly the scalar type in most cases. So Mark, i'm ok
with this, no need to worry anymore about what i said.

Cheers,
Benoit