Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arrange passing -Wall, check whether used compiler flags are supported #58

Merged
merged 2 commits into from Jul 29, 2019

Conversation

@aperezdc
Copy link
Member

commented Jul 25, 2019

The first commit arranges passing -Wall to the compiler, checking whether compiler flags are supported before adding them to the set of used ones, and using CMake's built-in C/C++ standard selection smarts. The second adds final to declarations to avoid a compiler warning (and it is good practice, too).

cmake: Pass -Wall and check compiler flags are supported
Adds checks to ensure that command line flags to be passed to the C/C++
compilers are supported before adding them to the list of flags. This
also leaves to CMake the responsibility of determining which compiler
flags are needed to choose C++11/C99, instead of passing -std=xyz,
because different compiler might use other options for this.

@aperezdc aperezdc requested review from carlosgcampos, zdobersek and clopez and removed request for carlosgcampos Jul 25, 2019

@carlosgcampos

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Does the compiler complain for not adding final to structs? it looks weird to me. I'm fine adding final for final classes that derive for another one, but I don't see how it helps for classes and structs with no parent.

@aperezdc

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

Does the compiler complain for not adding final to structs? it looks weird to me.

No, it doesn't… it just seemed a good idea to add final there because C++ allows “subclassing” a struct and also adding virtual methods to them. The idea is to avoid footguns: if someone subclasses a type, the compiler will bail because of the final and make us all think twice about how to do the subclassing properly.

I'm fine adding final for final classes that derive for another one, but I don't see how it helps for classes and structs with no parent.

For the ones with no parent, it makes the compiler forbid subclassing them.

Mark ViewBackend class as "final"
This hints the compiler that there will be no subclasses, so it
will stop warning that the class destructor is not virtual.

@aperezdc aperezdc force-pushed the cmake-wall-checkccflags branch from 99298a0 to 1903fc1 Jul 26, 2019

@aperezdc

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

Updated to add final only to the ViewBackend class.

@carlosgcampos, @mcatanzaro: WDYT about merging this now?

CMakeLists.txt Show resolved Hide resolved
@@ -54,7 +54,7 @@ class ClientBundle {
uint32_t initialHeight;
};

class ViewBackend : public WS::ExportableClient, public FdoIPC::MessageReceiver {
class ViewBackend final : public WS::ExportableClient, public FdoIPC::MessageReceiver {

This comment has been minimized.

Copy link
@mcatanzaro

mcatanzaro Jul 27, 2019

Collaborator

Oooh, multiple inheritance! WS::ExportableClient and FdoIPC::MessageReceiver still both need virtual destructors.

This comment has been minimized.

Copy link
@aperezdc

aperezdc Jul 27, 2019

Author Member

Not really. A method (or destructor) needs to be marked virtual to make sure that the compiler ends up calling the one at the end of the inheritance chain when applying the delete operator to a pointer typed with one of the superclasses.

In the case of ViewBackend, the deletion is done in this way:

auto* backend = reinterpret_cast<ViewBackend*>(data);
delete backend;

Here the compiler knows that we are deleting a ViewBackend (see the cast!), which will not ever have subclasses (when marked final). Therefore it knows that it always needs to use ViewBackend::~ViewBackend() for destruction, and that this is always safe because there cannot be a more concrete implementation.

This comment has been minimized.

Copy link
@aperezdc

aperezdc Jul 27, 2019

Author Member

Side note: If the destructor was marked virtual and at the same time the ViewBackend class is marked final, then the compiler would devirtualize the call—which effectively ends up choosing which version of the method/destructor will be used statically at compile time (thus skipping the dynamic vtable lookup at runtime).

This comment has been minimized.

Copy link
@mcatanzaro

mcatanzaro Jul 27, 2019

Collaborator

Here the compiler knows that we are deleting a ViewBackend (see the cast!), which will not ever have subclasses (when marked final). Therefore it knows that it always needs to use ViewBackend::~ViewBackend() for destruction, and that this is always safe because there cannot be a more concrete implementation.

I understand that, but if you ever change the way this ViewBackend instance is deleted, or if you ever create a different ViewBackend instance elsewhere and delete it from a parent class pointer, you leak the ViewBackend portion of the class, akin to slicing. This is much more of a footgun than leaving "final" off a struct declaration.

As a rule, any class that is derived from should have a virtual destructor. This is accepted C++ best-practice for decades now and is also the rule we use in WebKit.

This comment has been minimized.

Copy link
@aperezdc

aperezdc Jul 27, 2019

Author Member

There's only two ways in the way that the ViewBackend can be destroyed:

  • A subclass is added, deletion still done through a ViewBackend* pointer: the compiler will tell you to remove final, and then warn about the class needing a virtual destructor. No problem.
  • No subclass is added, but the destruction is done through a superclass pointer (WS::ExportableClient* or FdoIPC::MessageReceiver*): the compiler will tell you that you need a virtual destructor because the type of the pointer which delete is applied to may have subclasses. Again: no problem.

So there's no footgun now that we have compiler warnings enabled. How is the so-called “accepted best practice” not just cargo-culting in this case?

P.S: There is not such thing as “leaking” a slice of the memory area allocated for an object as a result from the wrong constructor being chosen, what happens is that some constructors are just not executed—if the code that does not run happens to dispose of resources those won't be properly disposed (of course: not cool), but the whole zone of memory allocated for the instance itself is always freed in full.

@carlosgcampos

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

Updated to add final only to the ViewBackend class.

@carlosgcampos, @mcatanzaro: WDYT about merging this now?

LGTM

@aperezdc aperezdc merged commit ebe0725 into master Jul 29, 2019

@aperezdc aperezdc deleted the cmake-wall-checkccflags branch Jul 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.