Merge pull request #742 from lsst/tickets/DM-45834

DM-45834: Fix C++17 deprecations and C++20 compilation

include/lsst/afw/image/Pixel.h

@@ -223,9 +223,14 @@ class Pixel : public detail::MaskedImagePixel_tag {
     // Logical operators.  We don't need to construct BinaryExpr for them
     // as efficiency isn't a concern.
     //
-    /// Return true iff two pixels are equal (in all three of image, mask, and variance)
-    template <typename T1>
-    friend bool operator==(Pixel const& lhs, T1 const& rhs) {
+    /// Return true if two pixels are equal (in all three of image, mask, and variance)
+    /// Fix C++20 compilation ambiguous overload problem by creating overloads
+    template <typename T1,typename T2, typename T3>
+    friend bool operator==(Pixel const& lhs, Pixel<T1,T2,T3> const& rhs) {
+        return lhs.image() == rhs.image() && lhs.mask() == rhs.mask() && lhs.variance() == rhs.variance();
+    }
+    template <typename T1,typename T2, typename T3>
+    friend bool operator==(Pixel const& lhs, SinglePixel<T1, T2, T3> const& rhs) {
         return lhs.image() == rhs.image() && lhs.mask() == rhs.mask() && lhs.variance() == rhs.variance();
     }
 

include/lsst/afw/typehandling/GenericMap.h

@@ -530,7 +530,7 @@ class GenericMap {
     // Assume that each operator() has the same return type; variant will enforce it
     /// @cond
     template <class Visitor>
-    using _VisitorResult = std::result_of_t<Visitor && (K&&, bool&)>;
+    using _VisitorResult = std::invoke_result_t<Visitor, K&&, bool&>;
     /// @endcond
 
     // No return value, const GenericMap

include/lsst/afw/typehandling/Storable.h

@@ -57,7 +57,7 @@ LSST_EXCEPTION_TYPE(UnsupportedOperationException, pex::exceptions::RuntimeError
  */
 class Storable : public table::io::Persistable {
 public:
-    virtual ~Storable() noexcept = 0;
+    ~Storable() noexcept override = 0;
 
     /**
      * Create a new object that is a copy of this one (optional operation).

include/lsst/afw/typehandling/test.h

@@ -58,8 +58,9 @@ namespace test {
 
 namespace {
 class SimpleStorable : public Storable {
+friend class ComplexStorable;
 public:
-    virtual ~SimpleStorable() = default;
+    ~SimpleStorable() override = default;
 
     std::shared_ptr<Storable> cloneStorable() const override { return std::make_unique<SimpleStorable>(); }
 
@@ -87,17 +88,21 @@ class ComplexStorable final : public SimpleStorable {
 
     std::size_t hash_value() const noexcept override { return std::hash<double>()(storage); }
 
-    // Warning: violates both substitution and equality symmetry!
+    // Fix violation of both substitution and equality symmetry
+    // as the code was broken on gcc with C++20 but passed on clang
     bool equals(Storable const& other) const noexcept override {
-        auto complexOther = dynamic_cast<ComplexStorable const*>(&other);
-        if (complexOther) {
+        if (auto complexOther = dynamic_cast<ComplexStorable const*>(&other)) {
             return this->storage == complexOther->storage;
-        } else {
-            return false;
         }
+        return SimpleStorable::equals(other);
     }
-    bool operator==(SimpleStorable const& other) const override { return this->equals(other); }
 
+    bool operator==(SimpleStorable const& other) const override {
+        if (auto complexOther = dynamic_cast<ComplexStorable const*>(&other)) {
+            return this->storage == complexOther->storage;
+        }
+        return SimpleStorable::operator==(other);
+    }
 private:
     double storage;
 };
@@ -107,7 +112,7 @@ std::string universalToString(T const& value) {
     std::stringstream buffer;
     buffer << value;
     return buffer.str();
-};
+}
 
 // Would make more sense as static constants in GenericFactory
 // but neither string nor Storable qualify as literal types
@@ -242,7 +247,7 @@ BOOST_TEST_CASE_TEMPLATE_FUNCTION(TestConstVisitor, GenericMapFactory) {
                 return universalToString(map->at(KEY6));
             default:
                 throw std::invalid_argument("Bad key found");
-        };
+        }
     };
     std::vector<std::string> expected;
     for (int key : mapKeys) {

tests/test_polymorphicValue.cc

@@ -112,7 +112,7 @@ BOOST_AUTO_TEST_CASE(Copy) {
     BOOST_CHECK(copy == original);
 
     // Independent copy?
-    static_cast<ComplexStorable&>(copy.get()) = 4.2;
+    dynamic_cast<ComplexStorable&>(copy.get()) = 4.2;
     BOOST_CHECK(copy != original);
 }
 
@@ -126,7 +126,7 @@ BOOST_AUTO_TEST_CASE(Move) {
     // changes to copy shouldn't affect original
     auto postMove = PolymorphicValue(original);
     BOOST_REQUIRE(original == postMove);
-    static_cast<ComplexStorable&>(copy.get()) = 4.2;
+    dynamic_cast<ComplexStorable&>(copy.get()) = 4.2;
     BOOST_CHECK(original == postMove);
 }
 
@@ -137,7 +137,7 @@ BOOST_AUTO_TEST_CASE(CopyAssign) {
     BOOST_CHECK(copy == original);
 
     // Independent copy?
-    static_cast<ComplexStorable&>(copy.get()) = 4.2;
+    dynamic_cast<ComplexStorable&>(copy.get()) = 4.2;
     BOOST_CHECK(copy != original);
 }
 
@@ -152,7 +152,7 @@ BOOST_AUTO_TEST_CASE(MoveAssign) {
     // changes to copy shouldn't affect original
     auto postMove = PolymorphicValue(original);
     BOOST_REQUIRE(original == postMove);
-    static_cast<ComplexStorable&>(copy.get()) = 4.2;
+    dynamic_cast<ComplexStorable&>(copy.get()) = 4.2;
     BOOST_CHECK(original == postMove);
 }
 
@@ -168,7 +168,7 @@ BOOST_AUTO_TEST_CASE(ImplicitConversion) {
     // Does modifying contents change holder?
     Storable& contents = holder;
     BOOST_REQUIRE(holder == PolymorphicValue(ComplexStorable(3.5)));
-    static_cast<ComplexStorable&>(contents) = 1.6;
+    dynamic_cast<ComplexStorable&>(contents) = 1.6;
     BOOST_CHECK(holder != PolymorphicValue(ComplexStorable(3.5)));
 }