Bug 755

As discussed on IRC. No time to explore further, so recording log below. Bug 716 looks like it is related.

[11:32] <zfmgpu> Hello :-)
[11:32] <zfmgpu> short question
[11:32] <zfmgpu> the comma initializer seems to not work
[11:33] <zfmgpu> because the assert is always throwing an error
[11:33] <zfmgpu> Eigen::Matrix3f m; m << 1, 2, 3,4, 5, 6,7, 8, 9; std::cout << m;
[11:33] <zfmgpu> throws an error in debug mode
[11:34] <zfmgpu> always , because the CommaInitializer class is destructed after the code m << 1 is executed (it returns a CommaInitializer class by value)
[11:35] <zfmgpu> is this correct behaviour or wrong?
[11:38] <jitseniesen> zfmgpu: that should work, and it does work on my computer. What compiler are you using and what version of Eigen?
[11:38] <zfmgpu> eigen 3.2
[11:38] <zfmgpu> gcc c++11
[11:38] <zfmgpu> gcc 4.7
[11:39] <zfmgpu> EIGEN_DEVICE_FUNC
[11:39] <zfmgpu>   inline ~CommaInitializer()
[11:39] <zfmgpu>   {
[11:39] <zfmgpu>     eigen_assert((m_row+m_currentBlockRows) == m_xpr.rows()
[11:39] <zfmgpu>          && m_col == m_xpr.cols()
[11:39] <zfmgpu>          && "Too few coefficients passed to comma initializer (operator<<)");
[11:39] <zfmgpu>   }
[11:40] <zfmgpu> isn't the destructor called after this call:
[11:40] <zfmgpu> template<typename Derived>
[11:40] <zfmgpu> inline CommaInitializer<Derived> DenseBase<Derived>::operator<< (const Scalar& s)
[11:40] <zfmgpu> {
[11:40] <zfmgpu>   return CommaInitializer<Derived>(*static_cast<Derived*>(this), s);
[11:40] <zfmgpu> }
[11:41] <zfmgpu> this creates a temporary, right? which is copied to the return value, the temp gets deleted -> which calls the dtor
[11:41] <ChriSopht> hm, it appears we relied on RVO at that point
[11:41] <zfmgpu> ok,
[11:41] <zfmgpu> not guaranteed in debug mode, hm....
[11:41] <ChriSopht> but that's definitely not good practice
[11:42] <zfmgpu> what is not good practice?
[11:42] <zfmgpu> the code
[11:42] <ChriSopht> on C++11 we could make a clean solution with proper move-constructors
[11:42] <zfmgpu> jeah
[11:42] <ChriSopht> not good practice: relying on compiler optimization to have working code
[11:43] <zfmgpu> but anyway the eigen_assert would stay in the constructor also with Move semantics?
[11:43] <ChriSopht> we do need a solution that is guaranteed to work in C++03 mode as well
[11:44] <ChriSopht> we can make the move constructor set a flag in the deconstructed class to mark it as cleanly destructed somehow (in C++11)
[11:46] <zfmgpu> jeah, that would work with c++11
[11:46] <zfmgpu> but what with c++03
[11:46] <zfmgpu> it works only if compiler uses RVO
[11:46] <zfmgpu> saddly :-)
[11:47] <zfmgpu> would it be possible to put the eigen assert somewhere else?
[11:47] <zfmgpu> move the assert into the finished call if one wants an assert
[11:48] <zfmgpu> would that be an option maybee
[11:48] <ChriSopht> I'm afraid that would lose many valid asserts
[11:49] <ChriSopht> we could only activate asserts after the first operator,() is called
[11:49] <ChriSopht> I think starting at that point it is guaranteed that we have only one object which returns itself by reference
[11:51] <ChriSopht> and it would only not catch (senseless) calls such as A<<1.0;
[11:52] <ChriSopht> and for C++11 we can make a clean, valid solution
[11:55] <jitseniesen> ChriSopht: I'm not very happy with your solution, but I can't think of anything better
[11:55] <ChriSopht> jitseniesen: to reproduce compile with -fno-elide-constructors (on gcc)
[12:00] <ChriSopht> zfmgpu: could you file a bug-report please?
[12:03] <ChriSopht> a slightly-hacked solution in C++03 mode would be to const_cast the source object in the copy-constructor and mark it as clean
[12:04] <ChriSopht> we can disable that in NDEBUG mode (but I don't think perfomance is an issue for that kind of I/O)
[12:06] <ChriSopht> I'm not sure, but I hope we can rely on that the compiler does not do completely unmotivated additional copies
[12:20] <jitseniesen> I guess that's how std::auto_ptr works, transfering ownership in operator=()
[12:20] <jitseniesen> that sounds better to me
[12:21] <jitseniesen> I agree that performance is not an issue
[12:21] <ChriSopht> yes, right. operator=() needs to be re-defined as well
[12:22] <jitseniesen> ah, you talked about copy constructor, not operator=
[12:23] <ChriSopht> and std::auto_ptr is a good example that it (guaranteeing that the constructor is called exactly once) works somehow
[12:23] <jitseniesen> instead of const-cast-ing, can't we declare the copy c'tor to take a CommaInitializer& instead of a const CommaInitializer& ?
[12:24] <ChriSopht> yes, right. I think that is also how auto_ptr does it
[12:47] <ChriSopht> instead of adding a new member, we could also set m_col=m_xpr.cols(); m_row=m_xpr.rows(); m_currentBlockRows = 0;
[12:47] <ChriSopht> if RVO is enabled, we will not even have a performance regression
[12:58] <ChriSopht> simply providing CommaInitializer(CommaInitializer&) is not sufficient, unfortunately
[12:59] <ChriSopht> std::auto_ptr has a helper-class auto_ptr_ref
[13:46] <ChriSopht> I managed to create a working version using a CommaInitializerRef helper class
[13:47] <ChriSopht> however, I'm afraid this is not removed by RVO