Use override for tabbox classes
AcceptedPublic

Authored by gladhorn on Oct 28 2018, 11:54 AM.

Details

Reviewers
romangg
Group Reviewers
KWin

Diff Detail

Repository
R108 KWin
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4265
Build 4283: arc lint + arc unit
gladhorn created this revision.Oct 28 2018, 11:54 AM
Restricted Application added a project: KWin. · View Herald TranscriptOct 28 2018, 11:54 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
gladhorn requested review of this revision.Oct 28 2018, 11:54 AM

So far I always said no to such changes as it doesn't really offer advantages for done code and just clutters the commit history.

So far I always said no to such changes as it doesn't really offer advantages for done code and just clutters the commit history.

I do think there is a bit advantage: When refactoring and a base classes' function gets renamed, any overriding virtuals will stop compiling, so this kind of change makes the code more future-proof.

romangg added a reviewer: KWin.EditedOct 29 2018, 9:20 AM
romangg added a subscriber: romangg.

Generally I agree with Martin on not doing large code touch ups for the sake of it in old code. But in this case the touch up is self-contained and small and using the override keyword is very useful for spotting problems in compilation phase. So +1 from me.

So far I always said no to such changes as it doesn't really offer advantages for done code and just clutters the commit history.

I do think there is a bit advantage: When refactoring and a base classes' function gets renamed, any overriding virtuals will stop compiling, so this kind of change makes the code more future-proof.

I fully understand the advantage of override. I do add it to new code. I'm questioning the advantage of adding it to old and done code. Nobody has touched these classes for years and there won't be any reason to do so for years. And even if one would touch it, it's as simple as doing the change to add override then.

My point against such a change is that we either need to do this for all of KWin or not at all. And for all of KWin I think it's a bad idea as it introduces a commit breaking any git blame. Would you +1 such a change in Qt changing all overriding methods to override?

So far I always said no to such changes as it doesn't really offer advantages for done code and just clutters the commit history.

I do think there is a bit advantage: When refactoring and a base classes' function gets renamed, any overriding virtuals will stop compiling, so this kind of change makes the code more future-proof.

I fully understand the advantage of override. I do add it to new code. I'm questioning the advantage of adding it to old and done code. Nobody has touched these classes for years and there won't be any reason to do so for years. And even if one would touch it, it's as simple as doing the change to add override then.

My point against such a change is that we either need to do this for all of KWin or not at all. And for all of KWin I think it's a bad idea as it introduces a commit breaking any git blame. Would you +1 such a change in Qt changing all overriding methods to override?

Yes, I would even approve this kind of change in Qt. Of course even better doing it in the whole repo indeed. But currently I'm interested in the TabBox and making that accessible to blind users. For me doing small cleanups is generally a way to get familiar with the code a bit - a nice junior job to recruit new people or get into a code base. In my opinion we should have tools that support us instead of making ourselves obey arbitrary things imposed by tools. Git blame works perfectly fine after cleanup changes, the only thing changed is that you need to run it twice, which is of course unfortunate, but hardly breaking it (git blame ... find which commit did the reformatting. git blame formatting_commit~ and you are there).
Random examples in qtbase where override was added, 0/Q_NULLPTR->nullptr etc: d0c159c8e306709629e3af0b83b464c5cea15754 9a15ac356cbbc25eeb48ac3a8fcbe5fb96dad5f5 0731c092d107415cc300be2209cd2bfb417950e9 5c57c66d98554f91ce34df387ffc2da18189cf2c
I could presumably find another hundred cleanup commits, they simply are needed to keep the code base fresh and aligned with modern C++.

romangg accepted this revision.Nov 2 2018, 8:13 AM

I think the pro arguments outweigh the contra argument of breaking git history. Imo it's also better to have such changes in a separate commit than together with other functional changes.

This revision is now accepted and ready to land.Nov 2 2018, 8:13 AM