fix: add explicit virtual destructor in order to fix linker issues

[Swiftb0y]

May fix #13755

[daschuer]

For my understanding, without explicit declaration, the implicit default destructor is not virtual. This is an issue when a class is deleted by its base class.
There are not much cases where this is desired so overriding the destructor is kind of mandatory.

Can we add a CI check for this?

[daschuer]

I have no explanation for the issue though, the AutoFileReloader is used as a member variable where this issue is not relevant.

And the ~QObject is virtual so this is even more not an issue here.

[daschuer]

Since 707a08f fixes the issue, can we close this?

[Swiftb0y]

Can we add a CI check for this?

I don't think so. I would expect the compiler to catch this by itself already.

And the ~QObject is virtual so this is even more not an issue here.

Yeah I agree. The implicit destructor was a red herring. Nevertheless, its probably better practice to have it declared and defined in the .cpp (because it can't benefit from the implicitly generated one in the header).

Since 707a08f fixes the issue, can we close this?

That commit is part of this PR. So closing PR doesn't make sense.

[daschuer]

So closing PR doesn't make sense.

Oh yes, I see.

autofilereloader.cpp is not used in mixxx-lib and can be removed here:

mixxx/CMakeLists.txt

Line 1150 in 707a08f

src/util/autofilereloader.cpp

Nevertheless, its probably better practice to have it declared and defined in the .cpp (because it can't benefit from the implicitly generated one in the header).

Why? For my understanding it is better to remove the destructor again and allow the Compiler to inline it and finally optimize it away.

[daschuer]

We normally don't have such comments, remove?

[Swiftb0y]

I would be confused on where this comes from (since the remaining files in the module all live in the qml directory), so I figured it would be better to make clear why this is necessary. I can remove if you disagree with the exception.

[ronso0]

FYI I'm using autoreloader in #13082

[Swiftb0y]

Thanks for the headsup. well then it needs to be linked in both anyways. That discussion is therefore resolved.

[Swiftb0y]

Why? For my understanding it is better to remove the destructor again and allow the Compiler to inline it and finally optimize it away.

It can't inline the dtor though because its virtual. It may if the class was declared final, but even then only in certain cirumstances. Since its a QObject it will also have to call the QObject dtor at some point, which for sure can't be inlined because the PIMPL pattern.

I would fully agree with your conclusion if this was TriviallyDestructible, but with QObjects, the very opposite is the case (they always have a virtual destructor). So the compiler will likely declare the function as inline in the header and let the linker deduplicate it. So in the binary, its effectively as if it was always in the translation unit. So the hope that the compiler can optimize much here doesn't apply.

I will try to reproduce in compiler explorer, but I don't have time right now.

[daschuer]

Thank you for explaining. LGTM

Our library layout is still not optimal, but let's merge this now make main build again.