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 640 - -Wshadow and -Wsign-conversion warnings
Summary: -Wshadow and -Wsign-conversion warnings
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: 3.3 (current stable)
Hardware: All All
: Normal Unknown
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 3.3
  Show dependency treegraph
 
Reported: 2013-08-07 13:32 UTC by David Evans
Modified: 2015-08-19 20:05 UTC (History)
4 users (show)



Attachments

Description David Evans 2013-08-07 13:32:35 UTC
Some warnings about shadowed variables:

TriangularSolverMatrix.h line 136 (shadows 118)
JacobiSVD.h line 395 (shadows 375)
RealQZ.h line 327 (shadows 318)

I also see a lot of warnings for -Wsign-conversion, e.g.

Memory.h line 805 (caused by 0xFFC00000 = unsigned int, not int. Could be fixed by swapping order of & and >>)

PacketMath.h lines 131, 136, 369 (similar issue, looks like the fix would be to explicitly cast)

IO.h line 61 (string [] operator expects size_t). Seems this particular case could be fixed by simplifying the loop:
size_t i = matSuffix.length();
while ((i--)>0 && matSuffix[i]!='\n')
{
  rowSpacer += ' ';
}

and so on…
Comment 1 Rick Mann 2014-03-18 23:24:26 UTC
I'd like to try to up the importance of fixing this bug. Recently in our own code we ran into a bug due to an improperly shadowed variable, so we turned on the warning. This resulted in hundreds of warnings from Eigen (mostly duplicates).

Now, it's possible to suppress these warnings by using -isystem, but it really would be better for Eigen to not shadow variables at all. It's easy to introduce errors when doing so, and one should always strive to be able to build with all warnings turned on and no warning messages emitted.

Xcode doesn't directly support -isystem, and as a result, CMake doesn't know how to add -isystem search paths when generating Xcode projects. I realize these are limitations of Xcode and CMake (especially since one CAN set Other C Flags in Xcode to include -isystem). But the initial assertion stands; good defensive coding precludes allowing shadowing.

Examples:

eigen/Eigen/src/Core/products/TriangularSolverMatrix.h:136:23: warning: declaration shadows a local variable [-Wshadow]
                Index s = IsLower ? i+1 : i-rs;
                      ^
eigen/Eigen/src/Core/products/TriangularSolverMatrix.h:118:19: note: previous declaration is here
            Index s  = IsLower ? k2+k1 : i+1;

eigen/Eigen/src/SVD/JacobiSVD.h:395:16: warning: declaration shadows a local variable [-Wshadow]
        Scalar z = abs(work_matrix.coeff(p,q)) / work_matrix.coeff(p,q);
               ^
eigen/Eigen/src/SVD/JacobiSVD.h:375:12: note: previous declaration is here
    Scalar z;
           ^
Comment 2 Christoph Hertzberg 2014-03-19 01:15:52 UTC
Gael started fixing many of these. It's quite likely that 3.3 will be (mostly) shadow free.
Comment 3 Rick Mann 2014-03-19 01:17:51 UTC
That's great news! You guys have a much better attitude than Boost. They refuse to fix their shadows.
Comment 4 Gael Guennebaud 2015-06-09 09:36:18 UTC
Fixed for clang: https://bitbucket.org/eigen/eigen/commits/3c6892351266/


-Wshadow is still bogus in gcc, so I'll wait their answer before making our code unnecessarily uglier:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57709
Comment 5 Gael Guennebaud 2015-06-09 11:26:36 UTC
(In reply to Gael Guennebaud from comment #4)
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57709

followup: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66472
Comment 6 Christoph Hertzberg 2015-08-19 19:40:10 UTC
The dev-branch should be essentially shadow-free.
I don't think this is worth back-porting to 3.2
Comment 7 Rick Mann 2015-08-19 20:05:13 UTC
Thank you. I don't think it needs to be backported, either. We can just as easily update to 3.3.

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