Re: [eigen] Eigen2: Assert failure in QR

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

2010/7/15 Keir Mierle <mierle@xxxxxxxxx>:
> This patch is due to Nick Lewycky.
>       Fix C++ conformance problems in stable eigen; discovered by clang.
>  * the 'template' keyword is required when the type is dependent on a
>    template argument. With 'template' we treat the next identifier as
>    a template method instead of a member variable and less-than
>    operator.

Cool, surely is an error we've often met and indeed GCC is missing
some of them, but in this line,

-  return this->cast<bool>().cast<int>().sum();
+  return this->cast<bool>().template cast<int>().sum();

it seems to me that the template keyword is also needed in the first
cast call, isn't it?

>  * ei_cache_friendly_product was never actually static. Remove the
>    bogus 'static' keyword. (Adding it properly caused link errors.)

oh OK, CoreInstantiations.cpp. This is disabled by default, useless,
only kept for "compatibility".

>  * sink the definition of template function ei_hypot after the
>    declarations of the functions it tries to call.

ok.

In these lines:

-                   : int(Lhs::Flags) & (RowMajorBit|SparseBit)
+                   : int(Lhs::Flags & (RowMajorBit|SparseBit))

I would much prefer:

+                   : int(Lhs::Flags) & int(RowMajorBit|SparseBit)

> Clang warns on default template argument
>  int StorageOrder = (unsigned)-1
> so cast the whole RHS as int which appears to have been the intent.

The patch you attached doesn't seem to do anything about that, right?


Keir: feel free to push once the above issues are resolved.

Benoit
> Keir
> On Thu, Jul 15, 2010 at 11:18 AM, Keir Mierle <mierle@xxxxxxxxx> wrote:
>>
>> I have another set of patches for Clang compatibility we should add before
>> 2.0.15.
>> Give me a few minutes to send it in.
>> Keir
>>
>> On Thu, Jul 15, 2010 at 11:13 AM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx>
>> wrote:
>>>
>>> Time to release 2.0.15 ? There's been one other important fix.
>>>
>>> As soon as someone confirms gcc 3.3 and msvc, i'm OK to release.
>>>
>>> The flurry of 2.0 fixes in the past 2 months is rather impressive, i
>>> guess it means that the number of users increased...
>>>
>>> Benoit
>>>
>>> 2010/7/15 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
>>> > oh, we probably fixed the bug at the very same time ;)
>>> >
>>> > indeed, it scary to have such a bug, fortunately unit test in 3.0 are
>>> > much better ;)
>>> >
>>> > gael
>>> >
>>> > On Thu, Jul 15, 2010 at 7:58 PM, Keir Mierle <mierle@xxxxxxxxx> wrote:
>>> >> Fix attached. How did this ever work before?
>>> >> Keir
>>> >>
>>> >> On Thu, Jul 15, 2010 at 9:55 AM, Sameer Agarwal
>>> >> <sameeragarwal@xxxxxxxxxx>
>>> >> wrote:
>>> >>>
>>> >>> Hi Guys,
>>> >>> Maps and QR in Eigen2 triggers an assert error, depending on whether
>>> >>> -DNDEBUG is used or not.
>>> >>>
>>> >>> eigen# g++ qr_test.cc -o qr1
>>> >>>
>>> >>> eigen# ./qr1
>>> >>> Assertion failed: (rows == this->rows()), function resize, file
>>> >>> Eigen/src/Core/Map.h, line 84.
>>> >>> Abort trap
>>> >>>
>>> >>> eigen# g++ qr_test.cc -DNDEBUG -o qr2
>>> >>> eigen# ./qr2
>>> >>>
>>> >>> This is against Eigen 2.0.14.
>>> >>> Sameer
>>> >>
>>> >>
>>> >
>>> >
>>> >
>>>
>>>
>>
>
>