New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Before reporting a bug, please make sure that your Eigen version is up-to-date!
Bug 397 - Implicit conversion loses integer precision: 'Index' (aka 'long') to 'int' when using Clang in Block.h
Summary: Implicit conversion loses integer precision: 'Index' (aka 'long') to 'int' wh...
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: 3.2
Hardware: x86 - 64-bit All
: High Internal Design
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on: 808
Blocks: 3.3
  Show dependency treegraph
 
Reported: 2011-12-21 01:22 UTC by shadewind
Modified: 2016-01-28 21:15 UTC (History)
5 users (show)



Attachments
Proposed patch to fix loss of Integer Precision warnings. (5.60 KB, patch)
2012-09-24 14:23 UTC, Tobias Wood
no flags Details | Diff
fixes for tricky long to int conversions (43.92 KB, patch)
2014-07-08 17:18 UTC, Gael Guennebaud
no flags Details | Diff

Description shadewind 2011-12-21 01:22:13 UTC
When using Eigen on Mac OS X in Xcode with default build settings, Clang gives the following warning for Block.h for row 340 and 341:

Implicit conversion loses integer precision: 'Index' (aka 'long') to 'int'

The obvious solution is to change m_outerStride to be of type Index, just as the return type of m_xpr.outerStride() and m_xpr.innerStride().
Comment 1 Gael Guennebaud 2011-12-23 23:01:45 UTC
Actually I don't understand why we store it, and why we don't simply implement Index outerStride() as:

      return internal::traits<Block>::HasSameStorageOrderAsXprType
                    ? m_xpr.outerStride()
                    : m_xpr.innerStride();

just like innerStride()
Comment 2 Tobias Wood 2012-09-19 19:18:27 UTC
After setting the C++ dialect in Xcode to C++11 I am receiving similar warnings in the following places (Clang is specific to the actual variable, I have marked this with ** **:

Assign.h:
Line 356 ::run(dst, src, **outer**);
PermutationMatrix.h:
Line 114: inline Index size() const { return **indices().size()**; };
Line 549: inline int rows() const { return **m_matrix.rows()**; };
Line 550: inline int rows() const { return **m_matrix.cols()**; };
Transpositions.h:
Line 81: inline Index size() const {return **indices.size(); };
PartialPivLU.h:
Line 104: m_p(**matrix.rows()**),
Line 105: m_rowsTranspositions(**matrix.rows()**);
Line 256: row_transpositions[k] = **row_of_biggest_in_col**;
Line 274: first_zero_pivot = **k**;
Line 345: first_zero_pivot = **k+ret**;
Line 394: m_rowsTranspositions.resize(**size**);
Comment 3 Tobias Wood 2012-09-24 14:23:34 UTC
Created attachment 296 [details]
Proposed patch to fix loss of Integer Precision warnings.

I'm new to Eigen, so I just went through the code trying to find suspicious uses of 'int' instead of traits::Index. Where a typedef was missing for traits::Index I added one. In one case this conflicted with a function argument already called Index, so I named it IndexType.
Comment 4 Gael Guennebaud 2014-07-08 17:18:10 UTC
Created attachment 474 [details]
fixes for tricky long to int conversions

The following changeset fixes many implicit conversions:

https://bitbucket.org/eigen/eigen/commits/2a7bf8832563/

however, there are still many which are more tricky to solve. The attached patch is a first attempt to fix the remaining ones. One issue is what to do when mixing expressions having different index types? This is typically the case of expressions storing array(s) of indices for which in most cases 'int' is preferred to limit the memory consumption: PermutationMatrix, Transpositions, and SparseMatrix. Another difficulty is that the 'Index' type of Matrix/Array is not templated.

As a result, for those objects, there is currently an ambiguity between the type used to store the indices and the 'Index' type used to define the dimensions, for the arguments of coeff(), and the likes. Currently, they are the same and thus we have many long-to-int-to-long conversions when, e.g., accessing the dimensions of a PermutationMatrix of int: PermutationMatrix<int>::rows() calls size() on the Matrix<int> object used to store the indices. This later returns a 'DenseIndex' while PermutationMatrix<int>::rows() returns an 'int' which is typically converted to a 'DenseIndex' in the callee code.

In the proposed patch, I introduced an 'IndexType' which should be read as 'StorageIndexType' for both PermutationMatrix and Transpositions. This type is only used for the storage and as the return type of coeff(). All other methods use 'DenseIndex'. This seems to be the cleanest possible solution. Unfortunately, this is also slightly changing the API of these classes, though such changes are unlikely to be noticeable.

Ideally, I would also make the same changes for SparseMatrix as the current workarounds to fix these conversions warnings are rather ugly.
Comment 5 Christoph Hertzberg 2014-07-16 16:07:40 UTC
I think your proposed API changes are ok. I doubt that any `regular user' works directly with PermutationMatrix.

Actually, it could make sense to be able to specify the IndexType as a parameter of pivoting decompositions (e.g., as optional second parameter after MatrixType) -- though I admit that there are barely cases where it would make a practical difference because usually the coefficients of the matrix take up magnitudes more of memory and we are not at the point yet where we can decompose matrices bigger than 2^31 in both dimensions.
Comment 6 Gael Guennebaud 2014-07-17 16:07:56 UTC
Patch applied.

I'll do the changes for sparse matrices directly in the evaluator branch.

https://bitbucket.org/eigen/eigen/commits/b92d6db282da/
Changeset:   b92d6db282da
User:        ggael
Date:        2014-07-17 13:34:26
Summary:     Bug 397: add a warning for 64 to 32 bit integer conversion and fix many of these warning by splitting the index type used for storage and as size/coefficient indexes in PermutationMatrix and Transpositions.
Comment 7 Gael Guennebaud 2016-01-28 21:15:51 UTC
Seems to be fixed by now.

Note You need to log in before you can comment on or make changes to this bug.