This bugzilla service is closed. All entries have been migrated to
Bug 755 - CommaInitializer bug in absence of RVO
Summary: CommaInitializer bug in absence of RVO
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - expression templates (show other bugs)
Version: unspecified
Hardware: All All
: High major
Assignee: Nobody
Depends on:
Blocks: 3.3
  Show dependency treegraph
Reported: 2014-03-07 16:40 UTC by Jitse Niesen
Modified: 2019-12-04 13:03 UTC (History)
4 users (show)

Proof-of-concept patch to avoid assertions if RVO is deactivated (3.46 KB, patch)
2014-03-07 17:31 UTC, Christoph Hertzberg
no flags Details | Diff
Simpler and unified workaround (1.75 KB, application/octet-stream)
2014-03-08 09:14 UTC, Gael Guennebaud
no flags Details

Description Jitse Niesen 2014-03-07 16:40:34 UTC
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
Comment 1 Christoph Hertzberg 2014-03-07 17:31:04 UTC
Created attachment 424 [details]
Proof-of-concept patch to avoid assertions if RVO is deactivated

Unfortunately it seems this patch generally prohibits RVO. In C++11 mode with rvalue references it takes the short-cut via the move constructor.
I accidentally worked on the 3.2 branch, but for the devel-branch it should work similarly.

I'm not sure if this has an impact on Bug 716, but I guess not.
Comment 2 Patrick Mihelich 2014-03-08 02:31:12 UTC
Awesome, I just ran into this and you guys are already on top of it :). We're using -fno-elide-constructors in our coverage build due to a gcc/gcov limitation:

Christoph's patch does work for me locally, with or without C++11. We are using C++11 in our build, so I'm happy to see move constructor support.
Comment 3 Gael Guennebaud 2014-03-08 09:14:00 UTC
Created attachment 425 [details]
Simpler and unified workaround

The previous patch does not work with clang. For some weird reasons, in the ctor from a CommaInitRef we get corrupted values. I tried to add a manual copy ctor in CommaInitRef but it is not called, If I make this copy ctor private, then it refuse to compile though.

Nevertheless, I think we can apply the same trick for c++11 or c++03, simply using a const_cast... This simpler workaround is still compatible with RVO in c++03 mode.
Comment 4 Christoph Hertzberg 2014-03-08 10:35:27 UTC
(In reply to comment #3)
> Nevertheless, I think we can apply the same trick for c++11 or c++03, simply
> using a const_cast... This simpler workaround is still compatible with RVO in
> c++03 mode.

Yes, simply doing a const_cast was what I also had in mind (comment 0, [12:03]). 
I guess it is safe enough here, so go ahead and push that simpler patch.
Comment 5 Christoph Hertzberg 2014-03-12 14:04:32 UTC
I applied Gael's workaround and also added some comments to the constructor:
Comment 6 Nobody 2019-12-04 13:03:33 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to'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:

Note You need to log in before you can comment on or make changes to this bug.