New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Before reporting a bug, please make sure that your Eigen version is up-to-date!
Bug 1587 - Make all Base-class constructors protected
Summary: Make all Base-class constructors protected
Status: REVIEWNEEDED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: 3.3 (current stable)
Hardware: All All
: Normal Internal Design
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-22 12:51 UTC by Christoph Hertzberg
Modified: 2018-12-14 10:46 UTC (History)
3 users (show)



Attachments
patch v1 (7.49 KB, patch)
2018-12-14 09:28 UTC, Gael Guennebaud
no flags Details | Diff

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.

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