Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix eigen build #1197

Merged
merged 4 commits into from
Aug 24, 2016
Merged

Fix eigen build #1197

merged 4 commits into from
Aug 24, 2016

Conversation

elcerdo
Copy link
Contributor

@elcerdo elcerdo commented Jul 12, 2016

Thanks a lot for contributing to DGtal, before submitting your PR, please fill up the description and make sure that all checkboxes are checked. Please remove these lines in your PR.

PR Description

your description here

Checklist

  • Unit-test of your feature with Catch.
  • Doxygen documentation of the code completed (classes, methods, types, members...)
  • Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
  • All continuous integration tests pass (Travis & appveyor)

@rolanddenis
Copy link
Member

rolanddenis commented Aug 8, 2016

I confirm that it solves the compilation issue with Eigen 3.3 (Ubuntu 16.04) but I was wondering if it is better to replace SparseMatrix::Index by SparseMatrix::StorageIndex (line 109 ) instead of modifying default index for SparseMatrix typedef, following instructions in http://eigen.tuxfamily.org/index.php?title=3.3#Index_typedef .

Finally, it seems that this modification (replacing Index by StorageIndex) doesn't work for previous Eigen versions (even previous 3.x releases, as pointed out in https://forum.kde.org/viewtopic.php?f=74&t=132256#p355171 ) so OK for your solution.

There is still warnings about variables shadowing class members (-Wshadow) but I think we can merge this PR ... are you ready ?

PS: sorry for the closed PR, it was a mistake...

@dcoeurjo
Copy link
Member

dcoeurjo commented Aug 9, 2016

Before the merge, @elcerdo would have to add an entry to the change log. This bug fix might be critical for compatibility with Eigen.

@dcoeurjo
Copy link
Member

@elcerdo can you please add a changelog entry?

@dcoeurjo dcoeurjo self-assigned this Aug 17, 2016
@dcoeurjo
Copy link
Member

Ping @elcerdo

@elcerdo
Copy link
Contributor Author

elcerdo commented Aug 24, 2016

added changelog entry.
@rolanddenis i didn't experience the -Wshadow warnings. which version of eigen are you using?
i am using eigen 3.2.8 under archlinux.

@@ -52,11 +52,14 @@
#include <iostream>
#if defined(__GNUG__)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wpragmas"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warnings can be ignored by adding a line here.

@rolanddenis
Copy link
Member

I get those warnings with Eigen 3.3-beta1 (shipped with Ubuntu 16.04, shown as 3.2.92) and Eigen 3.3-beta2 (shown as 3.2.93) with g++ (test with 5.4.0 and 4.9.3) in Debug mode and only WITH_EIGEN set (see https://gist.github.com/rolanddenis/65ecc38045e4fff22c54b4a12bf760e5 ).

No warnings when adding

#pragma GCC diagnostic ignored "-Wshadow"

or when using clang.

It's maybe related to this bug report http://eigen.tuxfamily.org/bz/show_bug.cgi?id=640 that also links to a gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66472

@dcoeurjo
Copy link
Member

Okay.. Let's merge this one and check later the warnings. (I'll add a more recent gcc in the buildbot to keep track of such issues).

@dcoeurjo
Copy link
Member

thanks ;)

@dcoeurjo dcoeurjo merged commit 5cb63d0 into DGtal-team:master Aug 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants