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

Wed Jan 23 10:04:49 GMT 2019

kossebau added a comment.

  In D18167#398076 <https://phabricator.kde.org/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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190123/c4152e3f/attachment.html>