IMHO a good idea, +1.

+1 to me too.

Since i have two +1 i'll commit this next saturday unless someone shouts in disagreement

This causes in KWin 500+ new warnings. Do you really think it's a good idea to spam all of KDE with new compiler warnings. KDE has an old code base. We cannot enable warnings for the way you developed C++ for 20 years.

In D18167#397020, @graesslin wrote:

This causes in KWin 500+ new warnings. Do you really think it's a good idea to spam all of KDE with new compiler warnings. KDE has an old code base. We cannot enable warnings for the way you developed C++ for 20 years.

These warnings are a good, thing, it's too easy to break an interface and never realize if you did not mark your functions as override.

Anyway, if you don't want the warnings just disable them for gcc, you already made the decision to disable it for clang, so just extend it to gcc.

For done code this warning is pointless and negative. I invite you to work with a code base like KWin where it is more important to have a working git blame than protection for theoretical problems. Nobody will be able to guarantee that a 500+ change to add override won't break. Human errors happen, nobody will be able to review something like that. Addressing this warning by adding override all over our legacy code base has a serious risk.

I'm seriously pissed that this is forced on us and we have to change code.

I'm totally fine with this warning for new code and new projects. But let projects opt in for it instead of forcing it on legacy code bases.

In D18167#397319, @graesslin wrote:

For done code this warning is pointless and negative. I invite you to work with a code base like KWin where it is more important to have a working git blame than protection for theoretical problems. Nobody will be able to guarantee that a 500+ change to add override won't break. Human errors happen, nobody will be able to review something like that. Addressing this warning by adding override all over our legacy code base has a serious risk.

I'm seriously pissed that this is forced on us and we have to change code.

I'm totally fine with this warning for new code and new projects. But let projects opt in for it instead of forcing it on legacy code bases.

@graesslin You, I and most if not all people are not happy if things change around us where we were fine with the status quo, so I understand you here. Just, please also understand the motivation for this change, which is about what is the recommended default for (KDE) projects. The alternatives would be to not change anything ever or to stick to the lowest common denominator. Would you agree those are less preferable solutions?
As a matter of fact, the (felt?) majority of maintained KDE projects meanwhile has been upgraded to the improved expressiveness of what the meanwhile rather old C++11 standards brings in terms of override (& nullptr). Especially as the upgrade usually can be done automated using compiler-powered tools, like clang-tidy, so the human factor is reduced by a good degree. My personal experience has been that one has rather found existing issues than introduced regressions. Especially with stone-age-old C++ codebases where lots of different lead developers had shifted designs in different directions, leaving some incomplete parts behind here and there accidentally when changing signatures of APIs.

So for KWin, if no-one dares to touch the old xcb(?) codebase (which actually means it is not "owned" any longer, but "owns" the developers), please now do the opt-out from the default by explicitly disabling that compiler flag for the legacy codebase. Again, the settings of the ECM KDE definition macros are not forced on people, just set the recommended defaults.

The human error exists as long as clang-tidy is not used. What I fear is that someone does a hand porting - we have seen several attempts to do that in KWin from various developers. If devs don't know and now fix the warnings, they can bring in human error.

Thus I suggest that those who think this should be the default for all projects by KDE do the work to run clang-tidy over the complete KDE code base and afterwards enable this warning.

I'm just not happy with the approach of breaking workflow without any discussion at all with the larger community. We have points in time where we can break things. E.g. the upcoming Qt 6. What I do not like is breaking in the middle of a release cycle without any coordination. Also I don't want to spend my very little spare time hunting behind what broke KWin build. I'm really not pleased about this from above attitude to break the compile of projects. It's one of the "dann macht euren Scheiss doch selbst" moments.

Btw. of course KWin is fully maintained - also the old xcb code. It's just not possible to review 500+ line changes with someone adding override. Furthermore we have here virtuals which nobody touched for 15 years, but git blame on them is super important. And if you wonder: we have 1555 override in KWin. We use the new possibilities. We just don't adjust the old code base which nobody touches. I'm sure you all will be the first one to yell if KWin breaks and renders your desktop unusable. Yes we have very strict requirements on stability. A change for the sake of change is not done in KWin.

So please revert this change and do a proper approach, talk to the community, ensure that this doesn't cause any new warnings and thus make it a useful new warning. In the current state it only causes work without any benefits for KWin.

Almost every project has already been gone over with clang-tidy.
Including kwin which was then force-pushed back by you. This was back in June 2017.
I've got little sympathy if we have a warning after explicitly reverting the fix to the warning.

I don't particularly buy the arguments against:

• It doesn't break git blame, as you need to know how to go quickly go through revisions to be able to use git blame in any real scenario anyway.
• If you use the argument that the warning is useless then by definition an incorrect override is equally useless and therefore harmless.

If kwin wants to do something special, (and given it does already for clang that seems like a non-issue, it would actually be removing code!), I disagree but won't stop it.

I see no reason to revert this.

In D18167#398076, @graesslin wrote:

The human error exists as long as clang-tidy is not used. What I fear is that someone does a hand porting - we have seen several attempts to do that in KWin from various developers. If devs don't know and now fix the warnings, they can bring in human error.

If I understand this paragraph correctly, introduction of unrelated changes, which can break code, is a human error, is it correct?

In D18167#398076, @graesslin wrote:

The human error exists as long as clang-tidy is not used. What I fear is that someone does a hand porting - we have seen several attempts to do that in KWin from various developers. If devs don't know and now fix the warnings, they can bring in human error.

Okay, this experience of yours hints to me why you appear to be a bit more sensitive about this change. Where myself I have never seen any issues related to moving to override usage, rather the opposite.
I also understand that you have an extra level of is-this-needed protection when it comes to kwin due to being a core product, which one could say has paid out so far given the stability we all enjoy with kwin. Just...

Thus I suggest that those who think this should be the default for all projects by KDE do the work to run clang-tidy over the complete KDE code base and afterwards enable this warning.

As said, it seems that most of the actively maintained codebase has already been moved to override usage (he, I am to "blame" for that as well) in the last two years or so. Which also could be seen as kind of silent support for seeing the non-use of override something to improve.

I'm just not happy with the approach of breaking workflow without any discussion at all with the larger community. We have points in time where we can break things. E.g. the upcoming Qt 6. What I do not like is breaking in the middle of a release cycle without any coordination. Also I don't want to spend my very little spare time hunting behind what broke KWin build. I'm really not pleased about this from above attitude to break the compile of projects.

I would not agree on the definition of "break". This change adds a warning by default. Same like if some new compiler version decides to be more nasty by default about issues it sees. Or methods being deprecated in some newer version of a library.

I would agree though to the point of this change being done very quickly in just a few days and without passing this a bit more visible by the eyes of the bigger developer community which relies on the defaults defined by ECM's KDE macros.
Just "build-system" and "frameworks" was not really sufficient here given the scope it effects, and only 3(?) days between proposal and commit was also a very rushy, ignoring that the major community might not be able to comment 24/7 on things. In a perfect organisation this change of default settings would have been exposed some more.
I do not think though this is "from above attitude", but more a "sane-to-me-and-my-circle-so-must-be-sane-to-everyone attitude" or a "afraid-of-potential-bikeshedding-discussion attitude"? Not healthy in any case.

So -1 on the execution of this change from me as well.

Though then in this very case, my own take is to be pragmatic and see that this change makes sense in the end and that any active KDE software projects which have code left which should not be upgraded to C++11 and more recent standards should simply on their side opt-out from this warning.

While talking about it, not sure what is the better approach, I have seen different cmake-based approaches:

string(REPLACE "-Wsuggest-override" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")

check_cxx_compiler_flag("-Wno-suggest-override" HAS_WNO_SUGGEST_OVERRIDE)

if (${HAS_WNO_SUGGEST_OVERRIDE})
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-suggest-override" )
endif()

What would cmake professionals use here?

In D18167#398343, @kossebau wrote:

only 3(?) days between proposal and commit was also a very rushy

Check your dates better please, it's 9 days

Though then in this very case, my own take is to be pragmatic and see that this change makes sense in the end and that any active KDE software projects which have code left which should not be upgraded to C++11 and more recent standards should simply on their side opt-out from this warning.

While talking about it, not sure what is the better approach, I have seen different cmake-based approaches:

string(REPLACE "-Wsuggest-override" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")

check_cxx_compiler_flag("-Wno-suggest-override" HAS_WNO_SUGGEST_OVERRIDE)

if (${HAS_WNO_SUGGEST_OVERRIDE})
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-suggest-override" )
endif()

What would cmake professionals use here?

The second one seems better to me.

In D18167#398076, @graesslin wrote:

The human error exists as long as clang-tidy is not used. What I fear is that someone does a hand porting - we have seen several attempts to do that in KWin from various developers. If devs don't know and now fix the warnings, they can bring in human error.

The nice thing about override is that the space for human error is *very* thin, it either compiles and then it's good or it doesn't compile and then it's bad.

In D18167#398360, @aacid wrote:

In D18167#398343, @kossebau wrote:

only 3(?) days between proposal and commit was also a very rushy

Check your dates better please, it's 9 days

Oops, indeed, my bad, somehow my eyes slipped one line from the data of the "Closed by commit" to the date of the announcement "will push next saturday". No coffee yet perhaps when I wrote that, the "(?)" hints I was at least unsure ;) But yes, blame taken.

Still, also 9 days I would consider a too short period for central setting changes. It has not been an urgent change.