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.

Sign up for GitHub

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

Correct options on Eigen::MappedSparseMatrix & adding MSVC C4127 suppression around Eigen includes. #3352

Merged
merged 3 commits into from
Oct 11, 2021
Merged

Correct options on Eigen::MappedSparseMatrix & adding MSVC C4127 suppression around Eigen includes. #3352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de09134
Adding MSVC C4127 suppression around Eigen includes.
Oct 11, 2021
1b72808
For MSVC 2015 only: also adding the C4127 suppression to test_eigen.cpp
Oct 11, 2021
f86c9dc
Copying original change from PR #3343, with extra line breaks to not …
Oct 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion include/pybind11/eigen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,23 @@

#include "numpy.h"

// The C4127 suppression was introduced for Eigen 3.4.0. In theory we could
// make it version specific, or even remove it later, but considering that
// 1. C4127 is generally far more distracting than useful for modern template code, and
// 2. we definitely want to ignore any MSVC warnings originating from Eigen code,
// it is probably best to keep this around indefinitely.
#if defined(_MSC_VER)
# pragma warning(push)
# pragma warning(disable: 4127) // C4127: conditional expression is constant
#endif

#include <Eigen/Core>
#include <Eigen/SparseCore>

#if defined(_MSC_VER)
# pragma warning(pop)
#endif

// Eigen prior to 3.2.7 doesn't have proper move constructors--but worse, some classes get implicit
// move constructors that break things. We could detect this an explicitly copy, but an extra copy
// of matrices seems highly undesirable.
Expand Down Expand Up @@ -559,7 +573,9 @@ struct type_caster<Type, enable_if_t<is_eigen_sparse<Type>::value>> {
if (!values || !innerIndices || !outerIndices)
return false;

value = Eigen::MappedSparseMatrix<Scalar, Type::Flags, StorageIndex>(
value = Eigen::MappedSparseMatrix<Scalar,
Type::Flags & (Eigen::RowMajor | Eigen::ColMajor),
StorageIndex>(
shape[0].cast<Index>(), shape[1].cast<Index>(), nnz,
outerIndices.mutable_data(), innerIndices.mutable_data(), values.mutable_data());

Expand Down
4 changes: 2 additions & 2 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,9 @@ set(PYBIND11_CROSS_MODULE_GIL_TESTS test_gil_scoped.py)
set(PYBIND11_EIGEN_REPO
"https://gitlab.com/libeigen/eigen.git"
CACHE STRING "Eigen repository to use for tests")
# This hash is for 3.3.8, using a hash for security reasons
# This hash is for 3.4.0, using a hash for security reasons
set(PYBIND11_EIGEN_VERSION
"dc252fbf00079ccab57948a164b1421703fe4361"
"929bc0e191d0927b1735b9a1ddc0e8b77e3a25ec"
CACHE STRING "Eigen version to use for tests")

# Check if Eigen is available; if not, remove from PYBIND11_TEST_FILES (but
Expand Down
3 changes: 3 additions & 0 deletions tests/test_eigen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
#include <pybind11/stl.h>

#if defined(_MSC_VER)
#if _MSC_VER < 1910 // VS 2015's MSVC
# pragma warning(disable: 4127) // C4127: conditional expression is constant
#endif
Comment on lines +16 to +18
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need this here? Isn't it suppressed in the include in pybind11? If that doesn't work, why are we suppressing it in the include?

Copy link
Collaborator

@Skylion007 Skylion007 Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@henryiii It works except for MSCV2015. So we need to do both.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Of course. 🤦 Okay.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For MSVC 2015 only the suppression in eigen.h is not sufficient:
https://github.com/pybind/pybind11/runs/3862647055?check_suite_focus=true

The suppression in eigen.h is for all MSVC versions, but only applies to the Eigen includes.
The suppression here is only for MSVC 2015, but global to this .cpp file.

# pragma warning(disable: 4996) // C4996: std::unary_negation is deprecated
#endif

Expand Down