| Summary: | -Wshadow and -Wsign-conversion warnings | ||
|---|---|---|---|
| Product: | Eigen | Reporter: | David Evans <davidje13> |
| Component: | Core - general | Assignee: | Nobody <eigen.nobody> |
| Status: | RESOLVED FIXED | ||
| Severity: | Unknown | CC: | chtz, gael.guennebaud, jacob.benoit.1, rmann |
| Priority: | Normal | ||
| Version: | 3.3 (current stable) | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 558 | ||
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;
^
Gael started fixing many of these. It's quite likely that 3.3 will be (mostly) shadow free. That's great news! You guys have a much better attitude than Boost. They refuse to fix their shadows. 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 (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 The dev-branch should be essentially shadow-free. I don't think this is worth back-porting to 3.2 Thank you. I don't think it needs to be backported, either. We can just as easily update to 3.3. -- GitLab Migration Automatic Message -- This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/640. |
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…