D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: zzag, davidedmundson, kossebau, graesslin, apol, vkrause, 
kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Albert Astals Cid]

aacid added a comment.


  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.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: zzag, davidedmundson, kossebau, graesslin, apol, vkrause, 
kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Albert Astals Cid]

aacid added a comment.


  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.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: zzag, davidedmundson, kossebau, graesslin, apol, vkrause, 
kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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?

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: zzag, davidedmundson, kossebau, graesslin, apol, vkrause, 
kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Vlad Zagorodniy]

zzag added a comment.


  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?

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: zzag, davidedmundson, kossebau, graesslin, apol, vkrause, 
kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[David Edmundson]

davidedmundson added a comment.


  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.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: davidedmundson, kossebau, graesslin, apol, vkrause, kde-frameworks-devel, 
kde-buildsystem, michaelh, ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Martin Flöser]

graesslin added a comment.


  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.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: kossebau, graesslin, apol, vkrause, kde-frameworks-devel, kde-buildsystem, 
michaelh, ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: kossebau, graesslin, apol, vkrause, kde-frameworks-devel, kde-buildsystem, 
michaelh, ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Martin Flöser]

graesslin added a comment.


  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.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: graesslin, apol, vkrause, kde-frameworks-devel, kde-buildsystem, michaelh, 
ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Albert Astals Cid]

aacid added a comment.


  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.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: graesslin, apol, vkrause, kde-frameworks-devel, kde-buildsystem, michaelh, 
ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Martin Flöser]

graesslin added a comment.


  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.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: graesslin, apol, vkrause, kde-frameworks-devel, kde-buildsystem, michaelh, 
ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Albert Astals Cid]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R240:7d73c6744f64: Move -Wsuggest-override -Wlogical-op to 
regular compiler settings (authored by aacid).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18167?vs=49189=49867

REVISION DETAIL
  https://phabricator.kde.org/D18167

AFFECTED FILES
  kde-modules/KDECompilerSettings.cmake
  kde-modules/KDEFrameworkCompilerSettings.cmake

To: aacid
Cc: apol, vkrause, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, 
bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Albert Astals Cid]

aacid added a comment.


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

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: apol, vkrause, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, 
bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Aleix Pol Gonzalez]

apol added a comment.


  +1 to me too.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: apol, vkrause, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, 
bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Volker Krause]

vkrause added a comment.


  IMHO a good idea, +1.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18167

To: aacid
Cc: vkrause, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Albert Astals Cid]

aacid created this revision.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
aacid requested review of this revision.

REVISION SUMMARY
  They really help making the code better so it's good to have all applications 
getting those warnings

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D18167

AFFECTED FILES
  kde-modules/KDECompilerSettings.cmake
  kde-modules/KDEFrameworkCompilerSettings.cmake

To: aacid
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns