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.

In D16479#350247, @graesslin wrote:

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.

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.

In D16479#350266, @gladhorn wrote:

In D16479#350247, @graesslin wrote:

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?

In D16479#351097, @graesslin wrote:

In D16479#350266, @gladhorn wrote:

In D16479#350247, @graesslin wrote:

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++.

In D16479#509552, @gladhorn wrote:

Looks like someone added override in the meantime :)

Yeah, we ran clang-tidy over kwin's code.