lmi
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[lmi] [PATCH] Use C++11 override keyword


From: Vadim Zeitlin
Subject: [lmi] [PATCH] Use C++11 override keyword
Date: Sun, 31 Jul 2016 23:58:27 +0200

 Hello,

 Here is the patch for the next step of C++11-ization which adds "override"
keywords in all places where they should be used, i.e. whenever a class
redeclares a virtual function of a base class:

                    https://github.com/vadz/lmi/pull/46

The patch is huge but should hopefully be uncontroversial as all the
changes are very similar. And it's worth doing this because accidentally
not overriding a virtual function that was meant to be overridden is a
relatively common error, so it's nice to let the compiler detect it,
especially if it happens because of a base class change (possibly in
wxWidgets code, although this should be very rare as we try to avoid such
silent breakages as much as possible).

 As with the previous nullptr patch, I tried to review the changes done by
clang-tidy to the best of my ability. I also tested the changes with clang
and gcc under Linux and MSVC and gcc under MSW and ran the test suite with
the latter and everything seems fine.


 The only niggling doubt I have is whether we should have kept the
"virtual" keyword too instead of dropping it. E.g. consider class
AboutDialog:

        class AboutDialog : public wxDialog {
                ...
                // wxDialog overrides.
                virtual int ShowModal();
                ...
        }

Current patch replaces the line above with

                int ShowModal() override;

but we could also possibly use

                virtual int ShowModal() override;

I've decided to drop "virtual" because it's redundant here (if the method
wasn't virtual, the use of "override" would be a compile-time error), but I
can imagine that you might prefer to keep it because it could be argued
that having an indication of the method being virtual at the beginning of
the line makes it easier to read at a glance. If you do prefer to keep it,
please let me know and I'll update the patch (this would require some extra
effort though, as I'd need to write a script doing it, I'm definitely not
updating ~600 lines by hand...).


 Please let me know if you have any other questions and thanks in advance
for applying this, I'll (probably) be waiting until it is done before
submitting the next C++11-ization patches.
VZ


reply via email to

[Prev in Thread] Current Thread [Next in Thread]