Run clang-tidy with modernize-use-override check
ClosedPublic

Authored by zzag on Jul 10 2019, 10:52 AM.

Details

Summary

Currently code base of kwin can be viewed as two pieces. One is very
ancient, and the other one is more modern, which uses new C++ features.

The main problem with the ancient code is that it was written before
C++11 era. So, no override or final keywords, lambdas, etc.

Quite recently, KDE compiler settings were changed to show a warning if
a virtual method has missing override keyword. As you might have already
guessed, this fired back at us because of that ancient code. We had
about 500 new compiler warnings.

A "solution" was proposed to that problem - disable -Wno-suggest-override
and the other similar warning for clang. It's hard to call a solution
because those warnings are disabled not only for the old code, but also
for new. This is not what we want!

The main argument for not actually fixing the problem was that git
history will be screwed as well because of human factor. While good git
history is a very important thing, we should not go crazy about it and
block every change that somehow alters git history. git blame allows to
specify starting revision for a reason.

The other argument (human factor) can be easily solved by using tools
such as clang-tidy. clang-tidy is a clang-based linter for C++. It can
be used for various things, e.g. fixing coding style(e.g. add missing
braces to if statements, readability-braces-around-statements check),
or in our case add missing override keywords.

Test Plan

Compiles.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
zzag created this revision.Jul 10 2019, 10:52 AM
Restricted Application added a project: KWin. · View Herald TranscriptJul 10 2019, 10:52 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Jul 10 2019, 10:52 AM
zzag added a comment.Jul 10 2019, 10:53 AM
This comment was removed by zzag.
zzag edited the summary of this revision. (Show Details)Jul 10 2019, 11:04 AM

I'm in favor of doing this. The problem with having this old style in code is not only warnings, but also one normally looks for examples in the code to write new code. Having old style in there leads to people using the old style for new code and then again the perpetuation of old style usage and/or prolonged reviews.

Since this change is so large maybe we want to wait till sprint though to do it in a coordinated fashion with potential other code style updates.

apol added a subscriber: apol.Jul 10 2019, 1:52 PM

I did a similar patch along the lines years ago and it was rejected in grounds of git history pollution, which I think is a bad reason to restrict standard c++ usage.

+1 from me

davidedmundson accepted this revision.Jul 21 2019, 3:13 PM
davidedmundson added a subscriber: davidedmundson.

Discussed at the meeting.

Ship it.

This revision is now accepted and ready to land.Jul 21 2019, 3:13 PM
This revision was automatically updated to reflect the committed changes.