Map allows to circumvent const-correctness.
Copied from email thread (">>" is Jim Bosch, ">" is Christoph Hertzberg): >> I'd be happy to put some effort into fixing this in Eigen 3.0, but as a >> relatively new face (well, I've been lurking a long time!) tackling a >> fairly widely-dispersed issue, I wanted to get some discussion going >> instead of just going out and making a giant patch. >> >> First off, here's what I think the solution should look like: >> >> - Matrix and Array (and other PlainObjectBase subclasses) should pretty >> much behave as they do currently. >> >> - Map, Block, Transpose, and anything else that does not own its data >> but supports direct access should come in both const and non-const >> versions (probably just partial specializations). The non-const version >> should be implicitly convertible to the const version. >> >> - Calling block() or transpose() on a const reference to Matrix should >> return a const Block or const Transpose(), etc. > > I think you mean sth like const_Block<...> or Block<const ..., ...>. The > const qualifier outside the class (as it is currently done) helps > little, as you can still construct/assign: > > const Block<...> a; > Block<...> b(a); > > This could be avoided by prohibiting the copy constructor etc, but this > would prohibit some legal copies. (Sorry for repeating this, I assume > you already know ...) > Yeah, that's is what I meant. >> What I think this implies: >> >> - MatrixBase, ArrayBase, DenseBase, DenseCoeffsBase, and EigenBase >> should also have partial specializations for const and non-const; the >> non-const version should probably inherit from the const version in some >> cases for code reuse, just adding non-const accessors and augmented >> assignment operators. >> >> - PlainObjectBase should inherit from a non-const base class. >> >> - Expressions that don't have direct access should inherit from the >> const MatrixBase or ArrayBase. >> >> - There should probably be a flag for constness on every Eigen class. >> >> >> I anticipate these changes would be fairly straightforward, but they are >> very extensive. I don't seen any easier way to get const-correctness in >> place in a consistent way, however, and I think doing it now will pay >> off in the long run. >> >> Any thoughts? > > I've also been thinking about this, my general idea was based on also > solving the direct-access-type unification > http://eigen.tuxfamily.org/bz/show_bug.cgi?id=58 > > The basic idea is to have a new (internal?) type direct_access with > template types for storage and for indexing. > > template<class Storage, class Indexing> > class direct_access : Indexing { > Storage storage; > public: > Storage::Scalar& operator(int i, int j) { > // not valid if Storage is a const-view: > return storage[Indexing::offset(i, j)]; > } > // etc ... > }; > > The Storage class could be either a dynamic array, a static array, a > pointer or a const pointer -- and the Storage class would maintain the > const-correctness (i.e. it's illegal to assign to the const pointer type). > > The Indexing type would calculate indexes to a relative offset and could > be zero-sized if everything is known at compile time. > Theoretically Indexing could also handle cases like compressed > triangular storage (though handling of implicit cases would need some > thought): > http://listengine.tuxfamily.org/lists.tuxfamily.org/eigen/2010/07/msg00198.html > Interesting idea with the storage classes - there's certainly a lot more flexibility there - but it seems like making them play nicely with alignment and vectorization could be difficult. And while your proposed direct_access and Storage classes may have to be const-aware, I think adding them is a problem that's mostly orthogonal to the const-correctness problem.
Created attachment 24 [details] First sample showing CRP issues and const correctness.
In principle I like the idea of returning TemplateExression< const PlainObject > but I am afraid there are some problems invovled. Code like this does not work: const PlainObject obj; TemplateExression< const PlainObject >& obj_expression = obj; The only thing you can do is const PlainObject obj; const TemplateExression< PlainObject >& obj_expression = obj; This is clearly a weakness of the CRP. Because the object itself does not know at compile time that it is const, we can not flag the Derived type properly. I see no way around this issue and we definitely need some ideas here. I attached a code example that shows this issue outside of Eigen since there we need to do a lot more but most things are mere spade work.
(In reply to comment #1) > >> Any thoughts? > > > > I've also been thinking about this, my general idea was based on also > > solving the direct-access-type unification > > http://eigen.tuxfamily.org/bz/show_bug.cgi?id=58 > > > > The basic idea is to have a new (internal?) type direct_access with > > template types for storage and for indexing. > > > > template<class Storage, class Indexing> > > class direct_access : Indexing { > > Storage storage; > > public: > > Storage::Scalar& operator(int i, int j) { > > // not valid if Storage is a const-view: > > return storage[Indexing::offset(i, j)]; > > } > > // etc ... > > }; > > > > The Storage class could be either a dynamic array, a static array, a > > pointer or a const pointer -- and the Storage class would maintain the > > const-correctness (i.e. it's illegal to assign to the const pointer type). > > > > The Indexing type would calculate indexes to a relative offset and could > > be zero-sized if everything is known at compile time. > > Theoretically Indexing could also handle cases like compressed > > triangular storage (though handling of implicit cases would need some > > thought): > > http://listengine.tuxfamily.org/lists.tuxfamily.org/eigen/2010/07/msg00198.html > > > > Interesting idea with the storage classes - there's certainly a lot more > flexibility there - but it seems like making them play nicely with alignment > and vectorization could be difficult. I don't think that there will be problems which haven't been solved yet e.g. for Map. > And while your proposed direct_access > and Storage classes may have to be const-aware, I think adding them is a > problem that's mostly orthogonal to the const-correctness problem. I'm aware that this merely can solve const-correctness for direct access types, not for Blocks of expressions (such as (A*B).block<...>(...)). (I did not check this recently but I think trying to assign to these is already (compile-time) prohibited.) It would however reduce a lot of duplicate code. Currently Map does essentially the same as a dynamic matrix -- except for memory allocation and the ability of resizing. (This part of the discussion could also be made in the thread for Bug 58)
(In reply to comment #3) > In principle I like the idea of returning > > TemplateExression< const PlainObject > > > but I am afraid there are some problems invovled. > > Code like this does not work: > > const PlainObject obj; > TemplateExression< const PlainObject >& obj_expression = obj; Sure, as PlainObject does not inherit from TemplateExpression<const ...>. If you want this syntax I guess the straight-forward solution would be to add another layer of inheritance: template <typename Derived> struct BaseClass : public BaseClass<const Derived> { Derived& derived() { return reinterpret_cast<Derived&>(*this); } void write() { derived().write(); } }; template <typename Derived> struct BaseClass<const Derived> { const Derived& derived() const { return reinterpret_cast<const Derived&>(*this); } float read() const { return derived().read(); } };
(In reply to comment #5) > Sure, as PlainObject does not inherit from TemplateExpression<const ...>. Right, what I said and we cannot even augment the base class' template parameter. > If you want this syntax I guess the straight-forward solution would be to add > another layer of inheritance: > > template <typename Derived> > struct BaseClass : public BaseClass<const Derived> > { > Derived& derived() { return reinterpret_cast<Derived&>(*this); } > void write() { derived().write(); } > }; > > template <typename Derived> > struct BaseClass<const Derived> { > const Derived& derived() const { return reinterpret_cast<const > Derived&>(*this); } > float read() const { return derived().read(); } > }; I see. That will work but implies enormous changes to the library since our inheritance tree is already quite deep. In order to prevent writing to a block expression resulting from another temporary expression (A*B).block<...>() = ... ; I think this can be fixed by the product return type being declared as OurSpecialProductReturnType<const MatrixXf> assuming A and B are e.g. MatrixXf. That should be it. Since all assignment calls are relayed through derived(), we'll be safe assuming MatrixXf, i.e. the underlying PlainObject is implemented const correct.
(In reply to comment #6) > I see. That will work but implies enormous changes to the library since our > inheritance tree is already quite deep. I wouldn't say that I endorse that solution either ... > In order to prevent writing to a block expression resulting from another > temporary expression > > (A*B).block<...>() = ... ; This is already impossible. But I admit that the error messages(!) generated by this: Eigen::Matrix3d A, B; (A * B).block<3,3>(0,0) = A; are not really helpful. My suggestion would be to make all direct access objects return a specialized direct access block (essentially a Map), and for all other expressions return a Block<Expression> as it is done right now, but which prohibits operator= (and non-const access to elements). Or are there any block expression for which operator= is possible? Maybe A.transpose().block() -- but transpose() called from a direct access object could actually also return a direct access type.
Created attachment 25 [details] How about fixing Map like this? We could fix Map's const-correctness issues by making the lib compatible to Map<const PlainObject>. For Map<typename PlainObject> we could then change the constructors such that they take const Scalar* when std::is_const<PlainObject>::value == std::true_type and to Scalar* else. In these cases, you cannot anymore map const data pointer to writable maps. I attached another example for that in case I am not clear enough.
(In reply to comment #8) > We could fix Map's const-correctness issues by making the lib compatible to > Map<const PlainObject>. yes, this is exactly I proposed in the initial thread about that issue a few months ago: "Issues regarding Quaternion-alignment and const Maps". I'm convinced this can be easily extended to other writable expressions such as Transpose, CwiseUnaryView, Block, etc. Also note that we have three different accessor base classes: 1 - DenseCoeffsBase<Derived, ReadOnlyAccessors> 2 - DenseCoeffsBase<Derived, WriteAccessors> 3 - DenseCoeffsBase<Derived, DirectAccessors> currently 3 inherits 2, which itself inherits 1 and the starting point (base class of DenseBase<Derived>) depends on the flags. To be the cleanest as possible, for a const object we could now have 3 inherits 1 directly though the use of such a "WriteAccessor" should be caught at the level of the derived class.
We all agree on the need for Map-to-const being a separate type, and Map<const T> seems like a natural choice. But that hasn't been discussed yet (it only is implicit in Gael's comment) is how to avoid letting that cause us to generate 2x more code in many circumstances. Example: matrix = some_map; matrix = some_const_map; Here, these 2 lines of code should use the same object code since the distinction between Map<T> and Map<const T> is irrelevant for rvalues. I realize that this could be treated as part of bug 58, but it would be nice if we could avoid making this (object duplication issue) worse than it currently is. This is the central difficulty here. As long as a function (here, operator=) is templated in the Derived type, it will be compiled separately for each Derived type. So the simplest fix would be to let it be a trivial inline wrapper calling another templated function which is NOT taking Derived as template parameter, instead is taking another type inferred from Derived but shedding the unneeded precision (i.e. not distinguishing between Map<T> and Map<const T>). One way to implement this would be to have a rvalue<T> metaprogram turning any expression into its rvalue variant. Like this (pseudo c++): template<typename T> struct rvalue { typename T type; } template<typename T> struct rvalue<Map<T> > { typename Map<const T> type; } We could then have operator= work like this: template<typename T> template<typename U> DenseBase<T>::operator=(const DenseBase<U>& other) { return assign_impl(typename rvalue<U>::type (other.derived()); } template<typename T> template<typename U> DenseBase<T>::assign_impl(const U& other) { ...actual code lives here... } In other words, we would really have 2x more expression types, but that wouldnt matter because we would isolate the object code generation from that. Of course operator= is just an example, however being an expression templates library we have relatively few places to check for this kind of issue, basically where we implement our loops, so besides operator= thats products, reductions, etc.
(In reply to comment #1) > > I've also been thinking about this, my general idea was based on also > > solving the direct-access-type unification > > http://eigen.tuxfamily.org/bz/show_bug.cgi?id=58 > > > > The basic idea is to have a new (internal?) type direct_access with > > template types for storage and for indexing. > > > > template<class Storage, class Indexing> > > class direct_access : Indexing { > > Storage storage; > > public: > > Storage::Scalar& operator(int i, int j) { > > // not valid if Storage is a const-view: > > return storage[Indexing::offset(i, j)]; > > } > > // etc ... > > }; > > > > The Storage class could be either a dynamic array, a static array, a > > pointer or a const pointer -- and the Storage class would maintain the > > const-correctness (i.e. it's illegal to assign to the const pointer type). > > > > The Indexing type would calculate indexes to a relative offset and could > > be zero-sized if everything is known at compile time. > > Theoretically Indexing could also handle cases like compressed > > triangular storage (though handling of implicit cases would need some > > thought): > > http://listengine.tuxfamily.org/lists.tuxfamily.org/eigen/2010/07/msg00198.html > > > > Interesting idea with the storage classes - there's certainly a lot more > flexibility there - but it seems like making them play nicely with alignment > and vectorization could be difficult. And while your proposed direct_access > and Storage classes may have to be const-aware, I think adding them is a > problem that's mostly orthogonal to the const-correctness problem. This discussion needs to happen on bug 58 :-) Your basic idea (direct_access class separating storage and indexing) seems like a good one but the crucial issue to solve will be to separate the object code generation from these template parameters so as to avoid redundant object code, as discussed in comment 10 here. The actual object code should only depend on the PlainObjectType, anything else is irrelevant.
(In reply to comment #11) > This discussion needs to happen on bug 58 :-) > Your basic idea (direct_access class separating storage and indexing) seems > like a good one but the crucial issue to solve will be to separate the object > code generation from these template parameters so as to avoid redundant object > code, as discussed in comment 10 here. The actual object code should only > depend on the PlainObjectType, anything else is irrelevant. Sorry, mistake here: at least the strides information is needed and is not part of the PlainObjectType.
The expression evaluator on why I've started working in bug 99 would (if it were accepted, which is not yet even discussed) nicely play the role of what I called rvalue<> in comment 10. Basically, if we go the expression evaluator route (bug 99), then expression types become completely dissociated from actual code generation, so there is no connection anymore between having more expression types and binary code bloat. Code would only get generated for each evaluator<> type on the RHS so we would only have to make sure that different expression types give the same evaluator<> type when possible. This is also possibly an approach to bug 58 although that one requires more thought (especially as evaluator is only for the RHS and bug 58 is also concerned about the LHS in assignments).
I just decided that bug 99 is for Eigen 3.1, not 3.0. I'm OK to go for the change described here, i.e. having T::Map(const Scalar*) giving a Map<const T>, even if that means redundant template instantiations, for 3.0. We can defer fixing this redundancy to Eigen 3.1. So, sorry if I delayed this discussion with my hesitations. I just wanted to make sure that this redundancy issue is on the radar and that we have a plan for it.
(In reply to comment #14) > I just decided that bug 99 is for Eigen 3.1, not 3.0. To be clear --- I just decided my own opinion, others (Gael...) might or might not agree with me, this can still be discussed! Though anyone wanting us to do bug 99 now would have to help with that!
Created attachment 43 [details] WIP patch (not currently compiling) Here's my current WIP patch, just 1 hour of work, not currently compiling, there are compile errors when trying to use Map<const T>, aside from that I *think* it's almost there.
Created attachment 47 [details] patch 99% ready It's essentially ready! // compile error: static assertion Map<Matrix3d>(const_ptr) // works, and expression doesn't have LvalueBit Map<Matrix3d const>(const_ptr) // compile error: static assertion Map<Matrix3d const>(const_ptr)(0,0) = 0; // compile error: the only operator() here returns a // const Scalar& so the compiler says 'not a lvalue'. Map<Matrix3d const>(const_ptr).cast_to_base_class()(0,0) = 0; // compile error: static assertion, and if you disable static asserts // it still fails to compile for previously mentioned reason Map<Matrix3d const>(const_ptr) = other_matrix; *** For now I've only tested the map and mapstride tests. I know that DenseCoeffsBase could be made a bit cleaner by isolating the stride accessors in a separate base class. TODO: the static methods e.g. MatrixXd::Map(...) have not yet been updated. Need to figure if they can be made convenient or if we should just tell people to do Map<MatrixXd>(...) / Map<MatrixXd const>(...).
(In reply to comment #17) > Created attachment 47 [details] > patch 99% ready > > It's essentially ready! Great, I'll check it out when it gets pushed :) > TODO: the static methods e.g. MatrixXd::Map(...) have not yet been updated. > Need to figure if they can be made convenient or if we should just tell people > to do Map<MatrixXd>(...) / Map<MatrixXd const>(...). What would be wrong with Matrix<>::Map(const double *) returning Map<const Matrix<> > and Matrix<>::Map(double *) returning Map<Matrix<> >? I think you can't keep full API compatibility here anyways, as const Map<...> is not the same thing as Map<const ...>, but in many cases Map is just used as a temporary so there wouldn't be a problem.
> > TODO: the static methods e.g. MatrixXd::Map(...) have not yet been updated. > > Need to figure if they can be made convenient or if we should just tell people > > to do Map<MatrixXd>(...) / Map<MatrixXd const>(...). > > What would be wrong with Matrix<>::Map(const double *) returning Map<const > Matrix<> > and Matrix<>::Map(double *) returning Map<Matrix<> >? Nothing is wrong with that, this is clearly the right solution, somehow I was overlooking it! Thanks! > I think you can't keep full API compatibility here anyways Indeed certainly not, as Map currently allows to circumvent const-correctness and we want these changes to disallow that.
Created attachment 51 [details] patch potentially ready, need to check tests Another update. I've had this patch for a few days but didn't understand why I couldn't get all tests to pass. It turns out that we have failing tests at the moment, see bug 128. I am now checking if this patch makes any currently passing test fail. We must get back to 100% passing tests. If this means that we need a build server hooked on hg pushes, so be it.
OK, my patch still has at least a build error in test/triangular.cpp
Pushed! So, good news / bad news time. Good news: const correctness errors are caught exactly like other const correctness errors: Map<const Foo> only has constructors taking a const Scalar*. Map<Foo (not const)> only has constructors taking Scalar*. That means that if you do const Scalar *p; Map<Foo>(p); You just get a compile error about 'no such constructor'. There is not even a need for a static assert here. Bad news: that means that I couldn't write a comprehensive unit test for this const correctness feature, because it wouldn't compile! I edited the test so it at least checks that LvalueBit is not set, but that's about it. So this could use some testing on your real world code! If you really want a unit test, the only way will be to have CMake scripts trying to compile tests and checking that they do fail to compile.
Created attachment 53 [details] Test case for clang This test case compiles fine with GCC, however it breaks with Clang. Any idea why? Tnx, Marton $ clang++ -v Apple clang version 2.0 (tags/Apple/clang-121.4) (based on LLVM 2.9svn) Target: x86_64-apple-darwin10 Thread model: posix $ clang++ -Iworkspace/eigen const_bug.cpp -o const_bug In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:308: workspace/eigen/Eigen/src/Core/products/GeneralBlockPanelKernel.h:539:3: warning: unknown attribute 'flatten' ignored [-Wunknown-attributes] EIGEN_FLATTEN_ATTRIB ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:36: workspace/eigen/Eigen/src/Core/util/Macros.h:164:45: note: instantiated from: #define EIGEN_FLATTEN_ATTRIB __attribute__((flatten)) ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:503:3: error: no member named 'YOU_ARE_TRYING_TO_WRITE_INTO_A_READ_ONLY_EXPRESSION' in 'Eigen::internal::static_assertion<32>' EIGEN_STATIC_ASSERT_LVALUE(Derived) ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:236: workspace/eigen/Eigen/src/Core/util/StaticAssert.h:189:7: note: instantiated from: EIGEN_STATIC_ASSERT(int(internal::traits<Derived>::Flags) & LvalueBit, \ ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:503:3: note: instantiated from: EIGEN_STATIC_ASSERT_LVALUE(Derived) ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:236: workspace/eigen/Eigen/src/Core/util/StaticAssert.h:190:27: note: instantiated from: YOU_ARE_TRYING_TO_WRITE_INTO_A_READ_ONLY_EXPRESSION) ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:534:93: note: in instantiation of function template specialization 'Eigen::DenseBase<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >::lazyAssign<Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here EIGEN_STRONG_INLINE static Derived& run(Derived& dst, const OtherDerived& other) { return dst.lazyAssign(other.derived()); } ^ workspace/eigen/Eigen/src/Core/Assign.h:574:59: note: in instantiation of member function 'Eigen::internal::assign_selector<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> >, Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, 0>::run' requested here return internal::assign_selector<Derived,OtherDerived>::run(derived(), other.derived()); ^ const_bug.cpp:10:42: note: in instantiation of function template specialization 'Eigen::MatrixBase<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >::operator=<Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here Eigen::Map<Eigen::Matrix2i>(p, 2, 2) = m; ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:44:35: warning: use of logical && with constant operand; switch to bitwise & or remove constant [-Wconstant-logical-operand] JointAlignment = DstIsAligned && SrcIsAligned ? Aligned : Unaligned ^ ~~~~~~~~~~~~ workspace/eigen/Eigen/src/Core/Assign.h:511:68: note: in instantiation of template class 'Eigen::internal::assign_traits<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> >, Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here internal::assign_impl<Derived, OtherDerived, int(SameType) ? int(internal::assign_traits<Derived, OtherDerived>::Traversal) ^ workspace/eigen/Eigen/src/Core/Assign.h:534:93: note: in instantiation of function template specialization 'Eigen::DenseBase<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >::lazyAssign<Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here EIGEN_STRONG_INLINE static Derived& run(Derived& dst, const OtherDerived& other) { return dst.lazyAssign(other.derived()); } ^ workspace/eigen/Eigen/src/Core/Assign.h:574:59: note: in instantiation of member function 'Eigen::internal::assign_selector<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> >, Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, 0>::run' requested here return internal::assign_selector<Derived,OtherDerived>::run(derived(), other.derived()); ^ const_bug.cpp:10:42: note: in instantiation of function template specialization 'Eigen::MatrixBase<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >::operator=<Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here Eigen::Map<Eigen::Matrix2i>(p, 2, 2) = m; ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:283: workspace/eigen/Eigen/src/Core/MapBase.h:214:7: error: no member named 'YOU_ARE_TRYING_TO_USE_AN_INDEX_BASED_ACCESSOR_ON_AN_EXPRESSION_THAT_DOES_NOT_SUPPORT_THAT' in 'Eigen::internal::static_assertion<16>' EIGEN_STATIC_ASSERT_LINEAR_ACCESS(Derived) ^ workspace/eigen/Eigen/src/Core/MapBase.h:30:7: note: instantiated from: EIGEN_STATIC_ASSERT(int(internal::traits<Derived>::Flags) & LinearAccessBit, \ ^ workspace/eigen/Eigen/src/Core/MapBase.h:214:7: note: instantiated from: EIGEN_STATIC_ASSERT_LINEAR_ACCESS(Derived) ^ workspace/eigen/Eigen/src/Core/MapBase.h:31:27: note: instantiated from: YOU_ARE_TRYING_TO_USE_AN_INDEX_BASED_ACCESSOR_ON_AN_EXPRESSION_THAT_DOES_NOT_SUPPORT_THAT) ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:258: workspace/eigen/Eigen/src/Core/DenseCoeffsBase.h:499:7: note: in instantiation of member function 'Eigen::MapBase<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> >, 1>::coeffRef' requested here derived().coeffRef(index) = other.derived().coeff(index); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:193:5: note: in instantiation of function template specialization 'Eigen::DenseCoeffsBase<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> >, 1>::copyCoeff<Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here dst.copyCoeff(Index, src); ^ workspace/eigen/Eigen/src/Core/Assign.h:326:9: note: in instantiation of member function 'Eigen::internal::assign_LinearTraversal_CompleteUnrolling<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> >, Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, 4>::run' requested here ::run(dst, src); ^ workspace/eigen/Eigen/src/Core/Assign.h:512:82: note: in instantiation of member function 'Eigen::internal::assign_impl<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> >, Eigen::Matrix<int, 2, 2, 0, 2, 2>, 1, 2>::run' requested here : int(InvalidTraversal)>::run(derived(),other.derived()); ^ workspace/eigen/Eigen/src/Core/Assign.h:534:93: note: in instantiation of function template specialization 'Eigen::DenseBase<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >::lazyAssign<Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here EIGEN_STRONG_INLINE static Derived& run(Derived& dst, const OtherDerived& other) { return dst.lazyAssign(other.derived()); } ^ workspace/eigen/Eigen/src/Core/Assign.h:574:59: note: in instantiation of member function 'Eigen::internal::assign_selector<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> >, Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, 0>::run' requested here return internal::assign_selector<Derived,OtherDerived>::run(derived(), other.derived()); ^ const_bug.cpp:10:42: note: in instantiation of function template specialization 'Eigen::MatrixBase<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >::operator=<Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here Eigen::Map<Eigen::Matrix2i>(p, 2, 2) = m; ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:503:3: error: no member named 'YOU_ARE_TRYING_TO_WRITE_INTO_A_READ_ONLY_EXPRESSION' in 'Eigen::internal::static_assertion<32>' EIGEN_STATIC_ASSERT_LVALUE(Derived) ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:236: workspace/eigen/Eigen/src/Core/util/StaticAssert.h:189:7: note: instantiated from: EIGEN_STATIC_ASSERT(int(internal::traits<Derived>::Flags) & LvalueBit, \ ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:503:3: note: instantiated from: EIGEN_STATIC_ASSERT_LVALUE(Derived) ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:236: workspace/eigen/Eigen/src/Core/util/StaticAssert.h:190:27: note: instantiated from: YOU_ARE_TRYING_TO_WRITE_INTO_A_READ_ONLY_EXPRESSION) ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:274: workspace/eigen/Eigen/src/Core/PlainObjectBase.h:335:14: note: in instantiation of function template specialization 'Eigen::DenseBase<Eigen::Matrix<int, 2, 2, 0, 2, 2> >::lazyAssign<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >' requested here return Base::lazyAssign(other.derived()); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:534:93: note: in instantiation of function template specialization 'Eigen::PlainObjectBase<Eigen::Matrix<int, 2, 2, 0, 2, 2> >::lazyAssign<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >' requested here EIGEN_STRONG_INLINE static Derived& run(Derived& dst, const OtherDerived& other) { return dst.lazyAssign(other.derived()); } ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:274: workspace/eigen/Eigen/src/Core/PlainObjectBase.h:505:69: note: in instantiation of member function 'Eigen::internal::assign_selector<Eigen::Matrix<int, 2, 2, 0, 2, 2>, Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> >, 0, 0>::run' requested here return internal::assign_selector<Derived,OtherDerived,false>::run(this->derived(), other.derived()); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:275: workspace/eigen/Eigen/src/Core/Matrix.h:289:7: note: in instantiation of function template specialization 'Eigen::PlainObjectBase<Eigen::Matrix<int, 2, 2, 0, 2, 2> >::_set_noalias<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >' requested here Base::_set_noalias(other); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:259: workspace/eigen/Eigen/src/Core/DenseBase.h:389:14: note: in instantiation of function template specialization 'Eigen::Matrix<int, 2, 2, 0, 2, 2>::Matrix<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >' requested here return typename internal::eval<Derived>::type(derived()); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:297: workspace/eigen/Eigen/src/Core/IO.h:256:38: note: in instantiation of member function 'Eigen::DenseBase<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >::eval' requested here return internal::print_matrix(s, m.eval(), EIGEN_DEFAULT_IO_FORMAT); ^ const_bug.cpp:11:15: note: in instantiation of function template specialization 'Eigen::operator<<<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >' requested here std::cout << Eigen::Map<Eigen::Matrix2i>(p, 2, 2) << std::endl; ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:109:46: warning: use of logical && with constant operand; switch to bitwise & or remove constant [-Wconstant-logical-operand] ? ( int(MayUnrollCompletely) && int(DstIsAligned) ? int(CompleteUnrolling) : int(NoUnrolling) ) ^ ~~~~~~~~~~~~~~~~~ workspace/eigen/Eigen/src/Core/Assign.h:511:68: note: in instantiation of template class 'Eigen::internal::assign_traits<Eigen::Matrix<int, 2, 2, 0, 2, 2>, Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >' requested here internal::assign_impl<Derived, OtherDerived, int(SameType) ? int(internal::assign_traits<Derived, OtherDerived>::Traversal) ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:274: workspace/eigen/Eigen/src/Core/PlainObjectBase.h:335:14: note: in instantiation of function template specialization 'Eigen::DenseBase<Eigen::Matrix<int, 2, 2, 0, 2, 2> >::lazyAssign<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >' requested here return Base::lazyAssign(other.derived()); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:534:93: note: in instantiation of function template specialization 'Eigen::PlainObjectBase<Eigen::Matrix<int, 2, 2, 0, 2, 2> >::lazyAssign<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >' requested here EIGEN_STRONG_INLINE static Derived& run(Derived& dst, const OtherDerived& other) { return dst.lazyAssign(other.derived()); } ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:274: workspace/eigen/Eigen/src/Core/PlainObjectBase.h:505:69: note: in instantiation of member function 'Eigen::internal::assign_selector<Eigen::Matrix<int, 2, 2, 0, 2, 2>, Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> >, 0, 0>::run' requested here return internal::assign_selector<Derived,OtherDerived,false>::run(this->derived(), other.derived()); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:275: workspace/eigen/Eigen/src/Core/Matrix.h:289:7: note: in instantiation of function template specialization 'Eigen::PlainObjectBase<Eigen::Matrix<int, 2, 2, 0, 2, 2> >::_set_noalias<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >' requested here Base::_set_noalias(other); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:259: workspace/eigen/Eigen/src/Core/DenseBase.h:389:14: note: in instantiation of function template specialization 'Eigen::Matrix<int, 2, 2, 0, 2, 2>::Matrix<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >' requested here return typename internal::eval<Derived>::type(derived()); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:297: workspace/eigen/Eigen/src/Core/IO.h:256:38: note: in instantiation of member function 'Eigen::DenseBase<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >::eval' requested here return internal::print_matrix(s, m.eval(), EIGEN_DEFAULT_IO_FORMAT); ^ const_bug.cpp:11:15: note: in instantiation of function template specialization 'Eigen::operator<<<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >' requested here std::cout << Eigen::Map<Eigen::Matrix2i>(p, 2, 2) << std::endl; ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:503:3: error: no member named 'YOU_ARE_TRYING_TO_WRITE_INTO_A_READ_ONLY_EXPRESSION' in 'Eigen::internal::static_assertion<32>' EIGEN_STATIC_ASSERT_LVALUE(Derived) ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:236: workspace/eigen/Eigen/src/Core/util/StaticAssert.h:189:7: note: instantiated from: EIGEN_STATIC_ASSERT(int(internal::traits<Derived>::Flags) & LvalueBit, \ ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:503:3: note: instantiated from: EIGEN_STATIC_ASSERT_LVALUE(Derived) ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:236: workspace/eigen/Eigen/src/Core/util/StaticAssert.h:190:27: note: instantiated from: YOU_ARE_TRYING_TO_WRITE_INTO_A_READ_ONLY_EXPRESSION) ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:274: workspace/eigen/Eigen/src/Core/PlainObjectBase.h:335:14: note: in instantiation of function template specialization 'Eigen::DenseBase<Eigen::Matrix<int, 2, 2, 0, 2, 2> >::lazyAssign<Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here return Base::lazyAssign(other.derived()); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:534:93: note: in instantiation of function template specialization 'Eigen::PlainObjectBase<Eigen::Matrix<int, 2, 2, 0, 2, 2> >::lazyAssign<Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here EIGEN_STRONG_INLINE static Derived& run(Derived& dst, const OtherDerived& other) { return dst.lazyAssign(other.derived()); } ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:274: workspace/eigen/Eigen/src/Core/PlainObjectBase.h:505:69: note: in instantiation of member function 'Eigen::internal::assign_selector<Eigen::Matrix<int, 2, 2, 0, 2, 2>, Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, 0>::run' requested here return internal::assign_selector<Derived,OtherDerived,false>::run(this->derived(), other.derived()); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:275: workspace/eigen/Eigen/src/Core/Matrix.h:296:7: note: in instantiation of function template specialization 'Eigen::PlainObjectBase<Eigen::Matrix<int, 2, 2, 0, 2, 2> >::_set_noalias<Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here Base::_set_noalias(other); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:297: workspace/eigen/Eigen/src/Core/IO.h:256:36: note: in instantiation of member function 'Eigen::Matrix<int, 2, 2, 0, 2, 2>::Matrix' requested here return internal::print_matrix(s, m.eval(), EIGEN_DEFAULT_IO_FORMAT); ^ const_bug.cpp:11:15: note: in instantiation of function template specialization 'Eigen::operator<<<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >' requested here std::cout << Eigen::Map<Eigen::Matrix2i>(p, 2, 2) << std::endl; ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:44:35: warning: use of logical && with constant operand; switch to bitwise & or remove constant [-Wconstant-logical-operand] JointAlignment = DstIsAligned && SrcIsAligned ? Aligned : Unaligned ^ ~~~~~~~~~~~~ workspace/eigen/Eigen/src/Core/Assign.h:511:68: note: in instantiation of template class 'Eigen::internal::assign_traits<Eigen::Matrix<int, 2, 2, 0, 2, 2>, Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here internal::assign_impl<Derived, OtherDerived, int(SameType) ? int(internal::assign_traits<Derived, OtherDerived>::Traversal) ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:274: workspace/eigen/Eigen/src/Core/PlainObjectBase.h:335:14: note: in instantiation of function template specialization 'Eigen::DenseBase<Eigen::Matrix<int, 2, 2, 0, 2, 2> >::lazyAssign<Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here return Base::lazyAssign(other.derived()); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:534:93: note: in instantiation of function template specialization 'Eigen::PlainObjectBase<Eigen::Matrix<int, 2, 2, 0, 2, 2> >::lazyAssign<Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here EIGEN_STRONG_INLINE static Derived& run(Derived& dst, const OtherDerived& other) { return dst.lazyAssign(other.derived()); } ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:274: workspace/eigen/Eigen/src/Core/PlainObjectBase.h:505:69: note: in instantiation of member function 'Eigen::internal::assign_selector<Eigen::Matrix<int, 2, 2, 0, 2, 2>, Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, 0>::run' requested here return internal::assign_selector<Derived,OtherDerived,false>::run(this->derived(), other.derived()); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:275: workspace/eigen/Eigen/src/Core/Matrix.h:296:7: note: in instantiation of function template specialization 'Eigen::PlainObjectBase<Eigen::Matrix<int, 2, 2, 0, 2, 2> >::_set_noalias<Eigen::Matrix<int, 2, 2, 0, 2, 2> >' requested here Base::_set_noalias(other); ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:297: workspace/eigen/Eigen/src/Core/IO.h:256:36: note: in instantiation of member function 'Eigen::Matrix<int, 2, 2, 0, 2, 2>::Matrix' requested here return internal::print_matrix(s, m.eval(), EIGEN_DEFAULT_IO_FORMAT); ^ const_bug.cpp:11:15: note: in instantiation of function template specialization 'Eigen::operator<<<Eigen::Map<Eigen::Matrix<int, 2, 2, 0, 2, 2>, 0, Eigen::Stride<0, 0> > >' requested here std::cout << Eigen::Map<Eigen::Matrix2i>(p, 2, 2) << std::endl; ^ In file included from const_bug.cpp:2: In file included from workspace/eigen/Eigen/Core:265: workspace/eigen/Eigen/src/Core/Assign.h:109:46: warning: use of logical && with constant operand; switch to bitwise & or remove constant [-Wconstant-logical-operand] ? ( int(MayUnrollCompletely) && int(DstIsAligned) ? int(CompleteUnrolling) : int(NoUnrolling) ) ^ ~~~~~~~~~~~~~~~~~ 5 warnings and 4 errors generated.
Crap. Only Map is fixed, now need to fix other lvalue expressions. Testcase: #include <iostream> #include <Eigen/Dense> using namespace std; using namespace Eigen; void modify_matrix(MatrixXd const & m) { Transpose<Eigen::MatrixXd> t = m.transpose(); t(0, 0) = 1.0; } int main() { MatrixXd m = MatrixXd::Zero(2,2); modify_matrix(m); std::cout << m << std::endl; }
Another one, I don't think it's connected though, as it breaks also with GCC: #include <Eigen/Core> using namespace Eigen; static const int N=1; int main() { float x = 3.0; Map<const Matrix<float,N,1> > c(&x); Map< Matrix<float,N,1> > m(&x); MatrixXf z(N,1); z.col(0) = c; //this breaks iff N=1 z.col(0) = m; //this compiles fine }
Pushed my new set of changes, so this should be fixed. Will post a discussion on the list ASAP. Only the Sparse module is not yet fixed, filed bug 140 about that. Sorry I've been busy recently so didn't take the time to answer the above recent comments. Will do ASAP too.
(In reply to comment #25) > Another one, I don't think it's connected though, as it breaks also with GCC: This one doesn't break with the current development branch.
There are still holes left, one of them I mentioned already: http://listengine.tuxfamily.org/lists.tuxfamily.org/eigen/2010/12/msg00072.html But I found two further ones when using Transpose and Diagonal and therefore took the liberty to reopen ... #include <Eigen/Eigen> using namespace Eigen; void foo(const Matrix3d &m){ Block<Matrix3d,3,3,false, false> b(m,0,0); b.setZero(); Transpose<Matrix3d> t(m); t.setZero(); Diagonal<Matrix3d> d(m); d.setZero(); }
(In reply to comment #28) > There are still holes left, one of them I mentioned already: > http://listengine.tuxfamily.org/lists.tuxfamily.org/eigen/2010/12/msg00072.html Sorry, forgot to reply to that. Non-direct-access expressions can still be lvalues, although one may discuss whether that can be the case here. But your test case below shows indeed that that's not specific to Block: > > But I found two further ones when using Transpose and Diagonal and therefore > took the liberty to reopen ... > > #include <Eigen/Eigen> > using namespace Eigen; > > void foo(const Matrix3d &m){ > Block<Matrix3d,3,3,false, false> b(m,0,0); > b.setZero(); > Transpose<Matrix3d> t(m); > t.setZero(); > Diagonal<Matrix3d> d(m); > d.setZero(); > } Thanks for the test case. It's really hard to fix this since we have to allow passing lvalue expressions here as temporaries i.e. by const reference. I will try a couple of ideas and report back.
OK, I just pushed a new patch (273c47794153), in particular it correctly prevents your test-case from compiling. The next thing to look at would be triangularView: need to check if a trivial modification of your example using a triangularView would again compile. In that case the fix would now be trivial: just see the part of this changeset affecting Diagonal.h, the same fix would apply there too. It's just a matter of using the new as_argument template helper to pass arguments to such expression constructors.
As I already mentioned, the big problem in this business is that we can't cover this with traditional unit tests, we'd need a whole new flavor of unit tests consisting of (cmake) script checking that programs DON'T compile.
Const-correctness is now covered by the new 'failtest' test suite, so regressions won't happen.
-- 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/54.