clang warning in SceneGraph: -Wnon-virtual-dtor

Question

clang warning in SceneGraph: -Wnon-virtual-dtor

asmaloney opened this issue 8 months ago · 10 comments

I build with warnings on (and warnings as errors), and I get the following (on 8203827):

src/Magnum/SceneGraph/SceneGraph.h:128: error: 'Magnum::SceneGraph::BasicMatrixTransformation3D' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor]

Apple clang version 16.0.0 (clang-1600.0.26.6)

Answer 1 · 2025-02-28T21:52:20.000Z

Huh, I don't recall getting this warning in any Clang builds (Clang 19 here, but neither on any Clangs that run on CIs or earlier versions that I remember building with). It also seems rather pointless because the transformation destructors are all protected, i.e. it's not possible to delete through a base class pointer, leading to potential resource leaks, which is what this warning is for I assume.

Nevertheless, does this patch make it stop complaining?

diff --git a/src/Magnum/SceneGraph/AbstractTransformation.h b/src/Magnum/SceneGraph/AbstractTransformation.h
index 60d44181c..1d6651a5b 100644
--- a/src/Magnum/SceneGraph/AbstractTransformation.h
+++ b/src/Magnum/SceneGraph/AbstractTransformation.h
@@ -76,7 +76,7 @@ template<UnsignedInt dimensions, class T> class AbstractTransformation {
         }
 
     protected:
-        ~AbstractTransformation() = default;
+        virtual ~AbstractTransformation() = default;
 
     #ifdef DOXYGEN_GENERATING_OUTPUT
     protected:

Answer 2 · 2025-02-28T23:04:05.000Z

Yes it does fix it. Now it's all building cleanly for me - nice work!

It's probably because I'm using "-Wall -Wextra -Wpedantic", etc..

Thanks Vladimír!

Answer 3 · 2025-02-28T23:30:21.000Z

I found https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102168 while looking around and ... changed my mind 😅

As they say, basically. Marking this destructor as virtual doesn't improve anything because this case is impossible to hit (unless you're trying really hard), and it increases size of the vtable for no reason. So I'd rather not mark it as such.

The second best solution is thus this, unless you want to follow their suggestion saying "This warning should not be used" and remove it from your warning set.

diff --git a/src/Magnum/SceneGraph/DualComplexTransformation.h b/src/Magnum/SceneGraph/DualComplexTransformation.h
index 081151a33..ed9f49b7a 100644
--- a/src/Magnum/SceneGraph/DualComplexTransformation.h
+++ b/src/Magnum/SceneGraph/DualComplexTransformation.h
@@ -36,6 +36,10 @@
 
 namespace Magnum { namespace SceneGraph {
 
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wnon-virtual-dtor"
+#endif
 /**
 @brief Two-dimensional transformation implemented using dual complex numbers
 
@@ -229,6 +233,9 @@ template<class T> class BasicDualComplexTransformation: public AbstractBasicTran
 
         Math::DualComplex<T> _transformation;
 };
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic pop
+#endif
 
 /**
 @brief Two-dimensional transformation for float scenes implemented using dual complex numbers
diff --git a/src/Magnum/SceneGraph/DualQuaternionTransformation.h b/src/Magnum/SceneGraph/DualQuaternionTransformation.h
index 5360336d1..d8591947e 100644
--- a/src/Magnum/SceneGraph/DualQuaternionTransformation.h
+++ b/src/Magnum/SceneGraph/DualQuaternionTransformation.h
@@ -36,6 +36,10 @@
 
 namespace Magnum { namespace SceneGraph {
 
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wnon-virtual-dtor"
+#endif
 /**
 @brief Three-dimensional transformation implemented using dual quaternions
 
@@ -257,6 +261,9 @@ template<class T> class BasicDualQuaternionTransformation: public AbstractBasicT
 
         Math::DualQuaternion<T> _transformation;
 };
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic pop
+#endif
 
 /**
 @brief Three-dimensional transformation for float scenes implemented using dual quaternions
diff --git a/src/Magnum/SceneGraph/MatrixTransformation2D.h b/src/Magnum/SceneGraph/MatrixTransformation2D.h
index 91a4befa3..0307083b6 100644
--- a/src/Magnum/SceneGraph/MatrixTransformation2D.h
+++ b/src/Magnum/SceneGraph/MatrixTransformation2D.h
@@ -36,6 +36,10 @@
 
 namespace Magnum { namespace SceneGraph {
 
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wnon-virtual-dtor"
+#endif
 /**
 @brief Two-dimensional transformation implemented using matrices
 
@@ -232,6 +236,9 @@ template<class T> class BasicMatrixTransformation2D: public AbstractBasicTransla
 
         Math::Matrix3<T> _transformation;
 };
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic pop
+#endif
 
 /**
 @brief Two-dimensional transformation for float scenes implemented using matrices
diff --git a/src/Magnum/SceneGraph/MatrixTransformation3D.h b/src/Magnum/SceneGraph/MatrixTransformation3D.h
index 779ac4a3a..c0ae6b1dd 100644
--- a/src/Magnum/SceneGraph/MatrixTransformation3D.h
+++ b/src/Magnum/SceneGraph/MatrixTransformation3D.h
@@ -36,6 +36,10 @@
 
 namespace Magnum { namespace SceneGraph {
 
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wnon-virtual-dtor"
+#endif
 /**
 @brief Three-dimensional transformation implemented using matrices
 
@@ -318,6 +322,9 @@ template<class T> class BasicMatrixTransformation3D: public AbstractBasicTransla
 
         Math::Matrix4<T> _transformation;
 };
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic pop
+#endif
 
 /**
 @brief Three-dimensional transformation for float scenes implemented using matrices
diff --git a/src/Magnum/SceneGraph/Object.h b/src/Magnum/SceneGraph/Object.h
index 1b5454d03..f6d8ada76 100644
--- a/src/Magnum/SceneGraph/Object.h
+++ b/src/Magnum/SceneGraph/Object.h
@@ -103,6 +103,10 @@ class documentation or @ref compilation-speedup-hpp for more information.
 @todo Consider using `mutable` for flags to make transformation computation
     available on const refs
 */
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wnon-virtual-dtor"
+#endif
 template<class Transformation> class Object: public AbstractObject<Transformation::Dimensions, typename Transformation::Type>, public Transformation
     #ifndef DOXYGEN_GENERATING_OUTPUT
     , private Containers::LinkedListItem<Object<Transformation>, Object<Transformation>>, private Containers::LinkedList<Object<Transformation>>
@@ -424,6 +428,9 @@ template<class Transformation> class Object: public AbstractObject<Transformatio
            causing the Object instance to be put in a multiple of 16 bytes
            even, instead of 8). */
 };
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic pop
+#endif
 
 }}
 
diff --git a/src/Magnum/SceneGraph/RigidMatrixTransformation2D.h b/src/Magnum/SceneGraph/RigidMatrixTransformation2D.h
index 01eeceb65..c4805f909 100644
--- a/src/Magnum/SceneGraph/RigidMatrixTransformation2D.h
+++ b/src/Magnum/SceneGraph/RigidMatrixTransformation2D.h
@@ -37,6 +37,10 @@
 
 namespace Magnum { namespace SceneGraph {
 
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wnon-virtual-dtor"
+#endif
 /**
 @brief Two-dimensional rigid transformation implemented using matrices
 
@@ -250,6 +254,9 @@ template<class T> class BasicRigidMatrixTransformation2D: public AbstractBasicTr
 
         Math::Matrix3<T> _transformation;
 };
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic pop
+#endif
 
 /**
 @brief Two-dimensional rigid transformation for float scenes implemented using matrices
diff --git a/src/Magnum/SceneGraph/RigidMatrixTransformation3D.h b/src/Magnum/SceneGraph/RigidMatrixTransformation3D.h
index 9fba360ab..bdcc9ec3c 100644
--- a/src/Magnum/SceneGraph/RigidMatrixTransformation3D.h
+++ b/src/Magnum/SceneGraph/RigidMatrixTransformation3D.h
@@ -37,6 +37,10 @@
 
 namespace Magnum { namespace SceneGraph {
 
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wnon-virtual-dtor"
+#endif
 /**
 @brief Three-dimensional rigid transformation implemented using matrices
 
@@ -335,6 +339,9 @@ template<class T> class BasicRigidMatrixTransformation3D: public AbstractBasicTr
 
         Math::Matrix4<T> _transformation;
 };
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic pop
+#endif
 
 /**
 @brief Three-dimensional rigid transformation for float scenes implemented using matrices
diff --git a/src/Magnum/SceneGraph/TranslationRotationScalingTransformation2D.h b/src/Magnum/SceneGraph/TranslationRotationScalingTransformation2D.h
index e1f60e552..3d1a016b5 100644
--- a/src/Magnum/SceneGraph/TranslationRotationScalingTransformation2D.h
+++ b/src/Magnum/SceneGraph/TranslationRotationScalingTransformation2D.h
@@ -37,6 +37,10 @@
 
 namespace Magnum { namespace SceneGraph {
 
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wnon-virtual-dtor"
+#endif
 /**
 @brief Two-dimensional transformation implemented using translation, rotation and scaling
 
@@ -237,6 +241,9 @@ template<class T> class BasicTranslationRotationScalingTransformation2D: public
            ... eh???) */
         Math::Vector2<T> _scaling = Math::Vector2<T>(T(1));
 };
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic pop
+#endif
 
 /**
 @brief Two-dimensional transformation for float scenes implemented using translation, rotation and scaling
diff --git a/src/Magnum/SceneGraph/TranslationRotationScalingTransformation3D.h b/src/Magnum/SceneGraph/TranslationRotationScalingTransformation3D.h
index b6f65494a..3872be236 100644
--- a/src/Magnum/SceneGraph/TranslationRotationScalingTransformation3D.h
+++ b/src/Magnum/SceneGraph/TranslationRotationScalingTransformation3D.h
@@ -37,6 +37,10 @@
 
 namespace Magnum { namespace SceneGraph {
 
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wnon-virtual-dtor"
+#endif
 /**
 @brief Three-dimensional transformation implemented using translation, rotation and scaling
 
@@ -326,6 +330,9 @@ template<class T> class BasicTranslationRotationScalingTransformation3D: public
            ... eh???) */
         Math::Vector3<T> _scaling = Math::Vector3<T>(T(1));
 };
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic pop
+#endif
 
 /**
 @brief Three-dimensional transformation for float scenes implemented using translation, rotation and scaling
diff --git a/src/Magnum/SceneGraph/TranslationTransformation.h b/src/Magnum/SceneGraph/TranslationTransformation.h
index 244d0cdeb..3ef992ded 100644
--- a/src/Magnum/SceneGraph/TranslationTransformation.h
+++ b/src/Magnum/SceneGraph/TranslationTransformation.h
@@ -37,6 +37,10 @@
 
 namespace Magnum { namespace SceneGraph {
 
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wnon-virtual-dtor"
+#endif
 /**
 @brief Translation-only transformation
 
@@ -125,6 +129,9 @@ class TranslationTransformation: public AbstractTranslation<dimensions, T, Trans
 
         VectorTypeFor<dimensions, TranslationType> _transformation;
 };
+#if defined(CORRADE_TARGET_GCC) || defined(CORRADE_TARGET_CLANG)
+#pragma GCC diagnostic pop
+#endif
 
 /**
 @brief Base transformation for two-dimensional scenes supporting translation

It's kind of nasty looking, but it silences the warnings for me. Can you confirm that it does for you as well, if you apply only this patch and revert the above? Thanks again!

Answer 4 · 2025-02-28T23:40:57.000Z

Well I'm fine with removing it from my warning set in the interests of not adding this fugliness to Magnum. (I have a standard warning set I use in various projects, but the way I am building/using Magnum at the moment means that it's OK to just remove it.)

Or, if you do want to try to "fix" it in case others run into it I'm happy to try out your patch.

Answer 5 · 2025-02-28T23:51:22.000Z

I'll sleep on it and decide :) Right now I'm leaning towards adding these pragmas to have a clean build log even with this (controversial?) warning enabled. Had to do a similar ugly thing for -Wdeprecated-literal-operator before, which cannot be fixed any other way because it'd make the code not build anymore on older compilers. So there's a precedent already.

So, if you can try out this patch so I'm sure I didn't miss any, that'd be great. I'll then likely push it in the morning. Thank you!

Answer 6 · 2025-03-01T00:38:18.000Z

I also needed to add the same wrapping in SceneGraph.h line 133:

#if defined( CORRADE_TARGET_GCC ) || defined( CORRADE_TARGET_CLANG )
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wnon-virtual-dtor"
#endif
      template <class> class BasicMatrixTransformation2D;
      template <class> class BasicMatrixTransformation3D;
      typedef BasicMatrixTransformation2D<Float> MatrixTransformation2D;
      typedef BasicMatrixTransformation3D<Float> MatrixTransformation3D;
#if defined( CORRADE_TARGET_GCC ) || defined( CORRADE_TARGET_CLANG )
#pragma GCC diagnostic pop
#endif

Answer 7 · 2025-03-01T00:41:19.000Z

Oh! It actually compiles cleanly if I only do that one wrapping in SceneGraph.h & not all the ones in your patch.

Answer 8 · 2025-03-01T11:27:07.000Z

Another attempt, this time without warning suppression:

diff --git a/src/Magnum/SceneGraph/DualComplexTransformation.h b/src/Magnum/SceneGraph/DualComplexTransformation.h
index 081151a33..eb2f07c4d 100644
--- a/src/Magnum/SceneGraph/DualComplexTransformation.h
+++ b/src/Magnum/SceneGraph/DualComplexTransformation.h
@@ -185,6 +185,8 @@ template<class T> class BasicDualComplexTransformation: public AbstractBasicTran
     protected:
         /* Allow construction only from Object */
         explicit BasicDualComplexTransformation() = default;
+        /* To silence -Wnon-virtual-dtor */
+        ~BasicDualComplexTransformation() = default;
 
     private:
         void doResetTransformation() override final { resetTransformation(); }
diff --git a/src/Magnum/SceneGraph/DualQuaternionTransformation.h b/src/Magnum/SceneGraph/DualQuaternionTransformation.h
index 5360336d1..2abc03c4c 100644
--- a/src/Magnum/SceneGraph/DualQuaternionTransformation.h
+++ b/src/Magnum/SceneGraph/DualQuaternionTransformation.h
@@ -209,6 +209,8 @@ template<class T> class BasicDualQuaternionTransformation: public AbstractBasicT
     protected:
         /* Allow construction only from Object */
         explicit BasicDualQuaternionTransformation() = default;
+        /* To silence -Wnon-virtual-dtor */
+        ~BasicDualQuaternionTransformation() = default;
 
     private:
         void doResetTransformation() override final { resetTransformation(); }
diff --git a/src/Magnum/SceneGraph/MatrixTransformation2D.h b/src/Magnum/SceneGraph/MatrixTransformation2D.h
index 91a4befa3..5a2669ff8 100644
--- a/src/Magnum/SceneGraph/MatrixTransformation2D.h
+++ b/src/Magnum/SceneGraph/MatrixTransformation2D.h
@@ -210,6 +210,8 @@ template<class T> class BasicMatrixTransformation2D: public AbstractBasicTransla
     protected:
         /* Allow construction only from Object */
         explicit BasicMatrixTransformation2D() = default;
+        /* To silence -Wnon-virtual-dtor */
+        ~BasicMatrixTransformation2D() = default;
 
     private:
         void doResetTransformation() override final { resetTransformation(); }
diff --git a/src/Magnum/SceneGraph/MatrixTransformation3D.h b/src/Magnum/SceneGraph/MatrixTransformation3D.h
index 779ac4a3a..665b4cde2 100644
--- a/src/Magnum/SceneGraph/MatrixTransformation3D.h
+++ b/src/Magnum/SceneGraph/MatrixTransformation3D.h
@@ -283,6 +283,8 @@ template<class T> class BasicMatrixTransformation3D: public AbstractBasicTransla
     protected:
         /* Allow construction only from Object */
         explicit BasicMatrixTransformation3D() = default;
+        /* To silence -Wnon-virtual-dtor */
+        ~BasicMatrixTransformation3D() = default;
 
     private:
         void doResetTransformation() override final { resetTransformation(); }
diff --git a/src/Magnum/SceneGraph/RigidMatrixTransformation2D.h b/src/Magnum/SceneGraph/RigidMatrixTransformation2D.h
index 01eeceb65..04495d216 100644
--- a/src/Magnum/SceneGraph/RigidMatrixTransformation2D.h
+++ b/src/Magnum/SceneGraph/RigidMatrixTransformation2D.h
@@ -210,6 +210,8 @@ template<class T> class BasicRigidMatrixTransformation2D: public AbstractBasicTr
     protected:
         /* Allow construction only from Object */
         explicit BasicRigidMatrixTransformation2D() = default;
+        /* To silence -Wnon-virtual-dtor */
+        ~BasicRigidMatrixTransformation2D() = default;
 
     private:
         void doResetTransformation() override final { resetTransformation(); }
diff --git a/src/Magnum/SceneGraph/RigidMatrixTransformation3D.h b/src/Magnum/SceneGraph/RigidMatrixTransformation3D.h
index 9fba360ab..bbf06558b 100644
--- a/src/Magnum/SceneGraph/RigidMatrixTransformation3D.h
+++ b/src/Magnum/SceneGraph/RigidMatrixTransformation3D.h
@@ -282,6 +282,8 @@ template<class T> class BasicRigidMatrixTransformation3D: public AbstractBasicTr
     protected:
         /* Allow construction only from Object */
         explicit BasicRigidMatrixTransformation3D() = default;
+        /* To silence -Wnon-virtual-dtor */
+        ~BasicRigidMatrixTransformation3D() = default;
 
     private:
         void doResetTransformation() override final { resetTransformation(); }
diff --git a/src/Magnum/SceneGraph/TranslationRotationScalingTransformation2D.h b/src/Magnum/SceneGraph/TranslationRotationScalingTransformation2D.h
index e1f60e552..1c9ff0f93 100644
--- a/src/Magnum/SceneGraph/TranslationRotationScalingTransformation2D.h
+++ b/src/Magnum/SceneGraph/TranslationRotationScalingTransformation2D.h
@@ -206,6 +206,8 @@ template<class T> class BasicTranslationRotationScalingTransformation2D: public
     protected:
         /* Allow construction only from Object */
         explicit BasicTranslationRotationScalingTransformation2D() = default;
+        /* To silence -Wnon-virtual-dtor */
+        ~BasicTranslationRotationScalingTransformation2D() = default;
 
     private:
         void doResetTransformation() override final { resetTransformation(); }
diff --git a/src/Magnum/SceneGraph/TranslationRotationScalingTransformation3D.h b/src/Magnum/SceneGraph/TranslationRotationScalingTransformation3D.h
index b6f65494a..2a77e03e5 100644
--- a/src/Magnum/SceneGraph/TranslationRotationScalingTransformation3D.h
+++ b/src/Magnum/SceneGraph/TranslationRotationScalingTransformation3D.h
@@ -286,6 +286,8 @@ template<class T> class BasicTranslationRotationScalingTransformation3D: public
     protected:
         /* Allow construction only from Object */
         explicit BasicTranslationRotationScalingTransformation3D() = default;
+        /* To silence -Wnon-virtual-dtor */
+        ~BasicTranslationRotationScalingTransformation3D() = default;
 
     private:
         void doResetTransformation() override final { resetTransformation(); }
diff --git a/src/Magnum/SceneGraph/TranslationTransformation.h b/src/Magnum/SceneGraph/TranslationTransformation.h
index 244d0cdeb..5a3c2776e 100644
--- a/src/Magnum/SceneGraph/TranslationTransformation.h
+++ b/src/Magnum/SceneGraph/TranslationTransformation.h
@@ -112,6 +112,8 @@ class TranslationTransformation: public AbstractTranslation<dimensions, T, Trans
     protected:
         /* Allow construction only from Object */
         explicit TranslationTransformation() = default;
+        /* To silence -Wnon-virtual-dtor */
+        ~TranslationTransformation() = default;
 
     private:
         void doResetTransformation() override final { resetTransformation(); }

This fixes it for me in SceneGraph. The same warning, when I enable it globally, however fires for basically all Platform::Application uses, and while for Clang I was able to work around it by putting final in basically all classes derived from these (which feels silly, because such instances are never deleted through a base pointer), on GCC that didn't help and warned about basically any use until I really make the destructors virtual. Which is stupid.

In conclusion, if this patch is enough to silence the warnings on your end, I'll commit that, because it's still relatively clean and doesn't feel too workaround-y. But for Platform::Application classes, in case those warn on your end as well, I unfortunately don't have a reasonable solution other than just disabling this warning.

Answer 9 · 2025-03-01T14:55:27.000Z

That patch works for me as well!

But for Platform::Application classes, in case those warn on your end as well,

I'm not using those - I'm rendering into a QOpenGLWidget - so the patch is fine for my use-case.

Thanks Vladimír!

Answer 10 · 2025-03-01T16:15:13.000Z

Pushed as cda3709, thanks!