DM-9932: Make afw classes RFC-209 compliant #312

kfindeisen · 2018-01-26T20:18:02Z

This PR changes all classes to have explicitly declared compiler-generated members, as adopted in RFC-209. I tried to make changes according to the following principles:

Do not change current behavior, even if the old behavior is arguably wrong. For example, a class with neither copy nor move constructors defined had both explicitly defaulted, while a class with a copy constructor but no move constructor got a move constructor that delegates to the copy constructor.
Follow the Rule of Three. In particular, I added explicit (defaulted) destructors wherever I added a copy or move constructor, and I added explicit members to classes where RFC-209 does not require them if they already had explicit destructors.
Keep definitions locations consistent. Members were explicitly defaulted in the header file if most constructors were inline, and in the source file if most constructors were defined there.
Use C++11 idioms. Where compiler-generated members had manually implemented defaults, I replaced them with the default keyword.

TallJimbo

Looks great!

I've made a number of small suggestions, mostly on ways in which you could go a bit further to clean things up a bit more. Feel free to ignore anything you think is out of scope, especially the areas where I'm advocating something that's not as precisely backwards-compatible as everything else you've done.

TallJimbo · 2018-01-27T02:08:02Z

include/lsst/afw/geom/AffineTransform.h

+    AffineTransform(AffineTransform const &) = default;
+    AffineTransform(AffineTransform &&) = default;
+    ~AffineTransform() = default;
+


I think assignment can be defaulted for this class, too; the implementation later in the class declaration should be equivalent to that.

TallJimbo · 2018-01-27T02:11:34Z

include/lsst/afw/geom/Box.h

@@ -101,6 +101,9 @@ class Box2I {

    /// Standard copy constructor.
    Box2I(Box2I const& other) : _minimum(other._minimum), _dimensions(other._dimensions) {}
+    // Delegate to copy-constructor for backwards-compatibility
+    Box2I(Box2I&& other) : Box2I(other) {}


I'm quite confident that defaults are fine for all compiler-generated member functions in this class.

I also understand if you'd prefer to just adhere strictly to your current guidelines, but I'll keep noting the ones I'm certain about as I go.

TallJimbo · 2018-01-27T02:12:13Z

include/lsst/afw/geom/Box.h

+    // Delegate to copy-constructor for backwards compatibility.
+    Box2D(Box2D&& other) : Box2D(other) {}
+
+    ~Box2D() = default;


Defaults for all compiler-generated functions would be fine for Box2D.

TallJimbo · 2018-01-27T02:17:34Z

include/lsst/afw/geom/CoordinateBase.h

+    CoordinateBase(CoordinateBase&&) = default;
+    CoordinateBase& operator=(CoordinateBase const&) = default;
+    CoordinateBase& operator=(CoordinateBase&&) = default;
+    virtual ~CoordinateBase() = default;


CoordinateBase's destructor should not be virtual; it's a CRTP base class, and we never hold its subclasses by a base class reference or pointer.

Note that virtual needs to be removed from the other specializations in this file as well.

That seems like a strange thing to guarantee. Why bother using inheritance, then? Keeping it virtual seems safer.

Sorry, I misspoke; we never delete its subclasses via a base class reference or pointer, which is much easier to guarantee. In this case, we also very rarely use the classes via a base class reference or pointer, but that's not an essential part of the pattern. For example, Eigen uses CRTP extensively, and the templated base classes are very much a part of the interface. It also doesn't define its destructors as virtual, because the base classes are by definition only used in contexts in which you can safely static_cast them a more derived class that can be inferred from their template arguments.

It's probably best to just say that because CRTP is essentially compile-time polymorphism, many of the usual rules of C++ inheritance that are derived from runtime polymorphism don't apply. I think a good case could be made that the implementations of Point and Extent don't actually benefit from these base classes much (they add a lot of boilerplate in exchange for reducing a bit of other code duplication), and I'd probably be supportive of a ticket to remove them, but we should absolutely not add a vtable to any of them.

I have no idea what you mean by "rules of C++ inheritance"; the principles of inheritance are language-independent. But it is true that none of the classes in question contain other virtual methods, so they can't really be used as interfaces anyway. I'll change the declarations.

TallJimbo · 2018-01-27T02:19:16Z

include/lsst/afw/geom/Extent.h

+    ExtentBase(ExtentBase &&) = default;
+    ExtentBase &operator=(ExtentBase const &) = default;
+    ExtentBase &operator=(ExtentBase &&) = default;
+    virtual ~ExtentBase() = default;


Should not be virtual (same reasons as CoordinateBase). Also other specializations in this file.

TallJimbo · 2018-01-27T03:19:58Z

src/formatters/MaskFormatter.cc

@@ -84,7 +84,7 @@ MaskFormatter<MaskPixelT>::MaskFormatter(std::shared_ptr<lsst::pex::policy::Poli
        : lsst::daf::persistence::Formatter(typeid(this)) {}

 template <typename MaskPixelT>
-MaskFormatter<MaskPixelT>::~MaskFormatter(void) {}
+MaskFormatter<MaskPixelT>::~MaskFormatter(void) = default;


Archaic void.

TallJimbo · 2018-01-27T03:31:36Z

src/formatters/MaskedImageFormatter.cc

@@ -110,7 +110,7 @@ MaskedImageFormatter<ImagePixelT, MaskPixelT, VariancePixelT>::MaskedImageFormat
        : lsst::daf::persistence::Formatter(typeid(this)) {}

 template <typename ImagePixelT, typename MaskPixelT, typename VariancePixelT>
-MaskedImageFormatter<ImagePixelT, MaskPixelT, VariancePixelT>::~MaskedImageFormatter(void) {}
+MaskedImageFormatter<ImagePixelT, MaskPixelT, VariancePixelT>::~MaskedImageFormatter(void) = default;


Archaic void.

TallJimbo · 2018-01-27T03:31:51Z

src/formatters/TanWcsFormatter.cc

+TanWcsFormatter& TanWcsFormatter::operator=(TanWcsFormatter const&) = default;
+TanWcsFormatter& TanWcsFormatter::operator=(TanWcsFormatter&&) = default;
+
+TanWcsFormatter::~TanWcsFormatter(void) = default;


Archaic void.

TallJimbo · 2018-01-27T03:32:03Z

src/formatters/WcsFormatter.cc

+WcsFormatter& WcsFormatter::operator=(WcsFormatter const&) = default;
+WcsFormatter& WcsFormatter::operator=(WcsFormatter&&) = default;
+
+WcsFormatter::~WcsFormatter(void) = default;


Archaic void.

TallJimbo · 2018-01-27T03:33:17Z

src/geom/Point.cc

+    for (int n = 0; n < N; ++n) {
+        this->_vector[n] = other[n];
+    }
+}


Should be able to default this instead (same later in this file); I can confirm that that would be equivalent for this class.

kfindeisen requested a review from TallJimbo January 26, 2018 20:18

TallJimbo approved these changes Jan 27, 2018

View reviewed changes

kfindeisen force-pushed the tickets/DM-9932 branch 3 times, most recently from a71567b to 83cc12c Compare January 31, 2018 22:13

kfindeisen added 12 commits January 31, 2018 14:13

Add explicit constructors to afw.cameraGeom.

b1a1122

Add explicit constructors to afw.coord.

3ab3652

Add explicit constructors to afw.detection.

5d4575d

Add explicit constructors to afw.formatters.

618f5d5

Add explicit constructors to afw.geom.

3c44c34

Add explicit constructors to afw.geom.polygon.

f037989

Add explicit constructors to afw.geom.ellipses.

1b28792

Add explicit constructors to afw.image.

85a1e30

Add explicit constructors to afw.math.

42988e1

Add explicit constructors to afw.math.detail.

4d0c15e

Add explicit constructors to afw.table.

32a4ada

Add explicit constructors to afw.table.io.

24a8078

kfindeisen force-pushed the tickets/DM-9932 branch from 83cc12c to 24a8078 Compare January 31, 2018 22:30

kfindeisen merged commit 24a8078 into master Feb 1, 2018

kfindeisen deleted the tickets/DM-9932 branch August 22, 2018 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-9932: Make afw classes RFC-209 compliant #312

DM-9932: Make afw classes RFC-209 compliant #312

kfindeisen commented Jan 26, 2018

TallJimbo left a comment

TallJimbo Jan 27, 2018

TallJimbo Jan 27, 2018

TallJimbo Jan 27, 2018 •

edited

TallJimbo Jan 27, 2018

kfindeisen Jan 29, 2018

TallJimbo Jan 29, 2018

kfindeisen Jan 29, 2018

TallJimbo Jan 27, 2018

TallJimbo Jan 27, 2018

TallJimbo Jan 27, 2018

TallJimbo Jan 27, 2018

TallJimbo Jan 27, 2018

TallJimbo Jan 27, 2018

DM-9932: Make afw classes RFC-209 compliant #312

DM-9932: Make afw classes RFC-209 compliant #312

Conversation

kfindeisen commented Jan 26, 2018

TallJimbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo Jan 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo Jan 27, 2018 •

edited