This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen

Bug 1587

Summary: Make all Base-class constructors protected
Product: Eigen Reporter: Christoph Hertzberg <chtz>
Component: Core - generalAssignee: Nobody <eigen.nobody>
Status: REVIEWNEEDED ---    
Severity: Internal Design CC: chtz, gael.guennebaud, jacob.benoit.1
Priority: Normal    
Version: 3.3 (current stable)   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
patch v1 none

Description Christoph Hertzberg 2018-08-22 12:51:02 UTC
Problems like the following could be avoided by making the copy constructors of all base-classes protected:
https://stackoverflow.com/questions/51958972/eigen-densebase-segmentation-fault

Actually, the user should never be able to directly construct a Base class, so all constructors should be protected.

I don't think this will break any valid/working code, but would expose some easy to make errors. Am I missing any valid use case of having public constructors here?
Comment 1 Gael Guennebaud 2018-08-22 21:03:15 UTC
I thought that has always been the case, but indeed it looks like that in addition to the default ctor we are missing the copy-ctor.
Comment 2 Gael Guennebaud 2018-12-13 20:05:08 UTC
Disabling the copy-ctor requires to implement copy-ctors for all derived classe, like:

    CwiseNullaryOp(const CwiseNullaryOp& other)
      : m_rows(other.m_rows), m_cols(other.m_cols), m_functor(other.m_functor)
    {}

I'm afraid that non-trivial copy-ctor will reduce the opportunities for compiler optimizations. This also means more boilerplate code and more opportunities for stupid bugs.
Comment 3 Christoph Hertzberg 2018-12-13 22:44:31 UTC
I would probably do it only for every *Base class which usually should be empty and don't cause overhead when copying -- and which is the more likely cause of errors. Also, I guess copying, e.g., a CwiseNullaryOp would not actually cause problems (except for the usual pitfalls like referring to objects which got destructed before usage).

Alternatively, we could define something like this only in c++11 mode (and have it empty otherwise):

#define EIGEN_HIDE_COPY_CTOR(name) protected: name(const name&) = default;
Comment 4 Gael Guennebaud 2018-12-14 09:28:31 UTC
Created attachment 908 [details]
patch v1

my bad, I mistakenly put one in a private section.

Here are my local changes for brief review. All unit tests are green.
Comment 5 Christoph Hertzberg 2018-12-14 10:46:50 UTC
Looks good to me (did not test it yet). I think DenseCoeffsBase, MapBase and PlainObjectBase could also get protected.
Comment 6 Nobody 2019-12-04 17:52:12 UTC
-- 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/1587.