Re: [eigen] Eigen2: Assert failure in QR

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

(Sorry Nick you'll receive this twice)

2010/7/15 Nick Lewycky <nlewycky@xxxxxxxxxx>:
>
>
> On 15 July 2010 13:54, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
>>
>> 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?
>
> You stumped me, so I asked the other clang developers and they're not sure
> either. You can add "template" after this-> too if you want to be more
> portable, but it's not clear whether or not it's really needed.
> As implemented, clang will find it because it knows that 'this' has type
> MatrixBase<T> which will never have a dependent base class. I spent a couple
> hours looking through the standard and wasn't able to find anything in there
> that would permit dropping the 'template' keyword in that case, even though
> I was sure there was...
> This is now llvm.org/PR7657 for us to figure it out. Thanks.

Thanks to you for looking into this. Since LLVM accepts the current
version, that's fine for now.

>> > 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?
>
> It does, that's the change to Lhs::Flags & RowMajorBit|SparseBit. Here's a
> larger snippet:
>    int StorageOrder = ei_is_part<Lhs>::value ? -1  // this is to solve
> ambiguous specializations
> -                   : int(Lhs::Flags) & (RowMajorBit|SparseBit)
> +                   : int(Lhs::Flags & (RowMajorBit|SparseBit))
> Note that the literal -1 there was never the problem, to my great confusion
> when I first encountered it!

OK, this is something we've often encountered: we need to cast enum
values to int in ?: to avoid a lot of warnings as they otherwise have
their own enum type. I wish there was custom-typed enums and/or
constexpr in c++98 :-(.

Benoit