D24826: Enforce 100 chars line width

[Roman Gilg]

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

REVISION SUMMARY
  The KDE Frameworks style recommended a 100 chars line limit but did not 
enforce
  it. Now with the clang-format file currently a somewhat arbitrary limit of 240
  chars is enforced.
  
  Instead enforce the recommendation of 100 chars per line.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  columnLimit

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

AFFECTED FILES
  kde-modules/clang-format.cmake

To: romangg, #frameworks, cullmann
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24826: Enforce 100 chars line width

[Christoph Cullmann]

cullmann added a comment.


  As explained in the thread on https://phabricator.kde.org/T11214, this will 
make the formatting ugly as hell, as if you have long method names, stuff is 
broken in arbitrary bad ways.
  I don't want to change that, if nobody can avoid the resulting breakage.

REPOSITORY
  R240 Extra CMake Modules

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

To: romangg, #frameworks, cullmann
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24826: Enforce 100 chars line width

[Roman Gilg]

romangg added a comment.


  In D24826#551280 , @cullmann wrote:
  
  > As explained in the thread on https://phabricator.kde.org/T11214, this will 
make the formatting ugly as hell, as if you have long method names, stuff is 
broken in arbitrary bad ways.
  >  I don't want to change that, if nobody can avoid the resulting breakage.
  
  
  Does it brutalize the line breaks always or only when the code line does not 
comply with the 100 chars limit? Or from the other side does `ColumnLimit: 240` 
mean that all "unnecessary" manual line breaks will be removed such that there 
are >200 chars lines now where before a document was only with <200? This would 
be even worse.

REPOSITORY
  R240 Extra CMake Modules

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

To: romangg, #frameworks, cullmann
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24826: Enforce 100 chars line width

[Christoph Cullmann]

cullmann added a comment.


  It is unfortunately very brutal and in-deed, it can create very long lines if 
you have matching bad constructs.
  
  One can play with stuff like a limit of 160 oder whatever to limit that, but 
as it brutally hard break and align stuff, with e.g. 100 for my tries, a lot of 
"OK" constructs that break this length got broken up in unreadable chunks.
  
  We play a long time with this at company and settled for 240 as some long 
lines are less worse than a lot of lalal->lalalala->lalala->llalala-> sequences 
broken up in X lines.

REPOSITORY
  R240 Extra CMake Modules

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

To: romangg, #frameworks, cullmann
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24826: Enforce 100 chars line width

[Christoph Cullmann]

cullmann added a comment.


  Perhaps easiest way to see what happens: apply it to one of your things and 
vary the value.

REPOSITORY
  R240 Extra CMake Modules

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

To: romangg, #frameworks, cullmann
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24826: Enforce 100 chars line width

[Vlad Zahorodnii]

zzag added a comment.


  I suggest to set ColumnLimit to 0 by default and allow projects to override 
it.

REPOSITORY
  R240 Extra CMake Modules

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

To: romangg, #frameworks, cullmann
Cc: zzag, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24826: Enforce 100 chars line width

[Allen Winter]

winterz added a comment.


  I am a long-time advocate of columnLimits; however, in our modern world of 
programming I think 100 is too short.
  
  240 may be a bit too long: in my personal coding style scripts I try to limit 
to 120.   even 120 is hard to achieve sometimes.
  I feel that 240 is a fair compromise.
  
  -1 for this patch

REPOSITORY
  R240 Extra CMake Modules

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

To: romangg, #frameworks, cullmann
Cc: winterz, zzag, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24826: Enforce 100 chars line width

[Christoph Cullmann]

cullmann added a comment.


  See the other task: columnlimit 0 leads to endless long lines and is a 
unusable default.

REPOSITORY
  R240 Extra CMake Modules

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

To: romangg, #frameworks, cullmann
Cc: winterz, zzag, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[Hannah von Reth]

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

REVISION SUMMARY
  Raise C from C89 ti C90 as C89 is not supported by the CMAKE flag
  
  
https://cmake.org/cmake/help/v3.16/prop_tgt/C_STANDARD.html#prop_tgt:C_STANDARD

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  cmake_c_standard

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

AFFECTED FILES
  kde-modules/KDECompilerSettings.cmake

To: vonreth
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[Hannah von Reth]

vonreth added reviewers: dfaure, cullmann.

REPOSITORY
  R240 Extra CMake Modules

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

To: vonreth, dfaure, cullmann
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[Hannah von Reth]

vonreth updated this revision to Diff 68475.
vonreth added a comment.


  Change message

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24841?vs=68473&id=68475

BRANCH
  cmake_c_standard

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

AFFECTED FILES
  kde-modules/KDECompilerSettings.cmake

To: vonreth, dfaure, cullmann
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[Christian Ehrlicher]

chehrlic added a comment.


  I would also use CMAKE_CXX_STANDARD_REQUIRED to make sure the compiler 
actually *can* c++11, otherwise the flag is only added when the compiler 
supports it. Should not make much difference nowadays but since it's in ECM...

REPOSITORY
  R240 Extra CMake Modules

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

To: vonreth, dfaure, cullmann
Cc: chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[Hannah von Reth]

vonreth added a comment.


  In D24841#551653 , @chehrlic wrote:
  
  > I would also use CMAKE_CXX_STANDARD_REQUIRED to make sure the compiler 
actually *can* c++11, otherwise the flag is only added when the compiler 
supports it. Should not make much difference nowadays but since it's in ECM...
  
  
  Good point.
  I also tried to investigate the supported C standard by msvc, they say C99 is 
there but still incomplete.

REPOSITORY
  R240 Extra CMake Modules

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

To: vonreth, dfaure, cullmann
Cc: chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[Hannah von Reth]

vonreth updated this revision to Diff 68478.
vonreth added a comment.


  CMAKE_CXX_STANDARD_REQUIRED

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24841?vs=68475&id=68478

BRANCH
  cmake_c_standard

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

AFFECTED FILES
  kde-modules/KDECompilerSettings.cmake

To: vonreth, dfaure, cullmann
Cc: chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[Hannah von Reth]

vonreth edited the summary of this revision.

REPOSITORY
  R240 Extra CMake Modules

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

To: vonreth, dfaure, cullmann
Cc: chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> KDECompilerSettings.cmake:208
> +set(CMAKE_C_STANDARD 90)
> +set(CMAKE_CXX_STANDARD_REQUIRED 11)
>  

That's not how it works.
You want

  set(CMAKE_CXX_STANDARD 11)
  set(CMAKE_CXX_STANDARD_REQUIRED TRUE)

Note however that these require CMake >= 3.1, while ECM says 
`cmake_minimum_required(VERSION 2.8.12 FATAL_ERROR)`. I do however thing that 
ECM *should* require 3.1.
I doubt it's been tested on 2.8.x in ages, and 3.1 is really a common 
requirement these days.

REPOSITORY
  R240 Extra CMake Modules

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

To: vonreth, dfaure, cullmann
Cc: chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[Albert Astals Cid]

aacid added a comment.


  If you want to increase cmake dependency, we should at least increase it to 
3.5
  
  https://repology.org/project/cmake/badges

REPOSITORY
  R240 Extra CMake Modules

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

To: vonreth, dfaure, cullmann
Cc: aacid, chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[David Faure]

dfaure added a comment.


  Works for me.

REPOSITORY
  R240 Extra CMake Modules

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

To: vonreth, dfaure, cullmann
Cc: aacid, chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[Hannah von Reth]

vonreth updated this revision to Diff 68486.
vonreth added a comment.


  Fix

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24841?vs=68478&id=68486

BRANCH
  cmake_c_standard

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

AFFECTED FILES
  kde-modules/KDECompilerSettings.cmake

To: vonreth, dfaure, cullmann
Cc: aacid, chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[Hannah von Reth]

vonreth added a comment.


  I guess we should do that in a separate review?

REPOSITORY
  R240 Extra CMake Modules

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

To: vonreth, dfaure, cullmann
Cc: aacid, chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[David Faure]

dfaure added a comment.


  Dunno, it's harder to justify in a standalone review :)
  "Increase dependency, just because" ;)

REPOSITORY
  R240 Extra CMake Modules

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

To: vonreth, dfaure, cullmann
Cc: aacid, chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[Hannah von Reth]

vonreth updated this revision to Diff 68488.
vonreth added a comment.


  - Raise CMake requirements to 3.5

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24841?vs=68486&id=68488

BRANCH
  cmake_c_standard

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

AFFECTED FILES
  CMakeLists.txt
  kde-modules/KDECompilerSettings.cmake
  tests/ECMAddAppIconTest/CMakeLists.txt
  tests/ECMAddTests/multi_tests/CMakeLists.txt
  tests/ECMAddTests/single_tests/CMakeLists.txt
  tests/ECMInstallIconsTest/CMakeLists.txt
  tests/ECMPoQmToolsTest/CMakeLists.txt
  tests/ECMQMLModules/CMakeLists.txt
  tests/ECMQtDeclareLoggingCategoryTest/CMakeLists.txt
  tests/ECMSetupVersionTest/old_header/CMakeLists.txt
  tests/ECMSetupVersionTest/old_header_abspath/CMakeLists.txt
  tests/ECMSetupVersionTest/old_simple/CMakeLists.txt
  tests/ECMSetupVersionTest/old_soversion/CMakeLists.txt
  tests/ECMSetupVersionTest/old_version_file/CMakeLists.txt
  tests/ECMSetupVersionTest/old_version_file_abspath/CMakeLists.txt
  tests/ECMSetupVersionTest/old_version_file_anynewer/CMakeLists.txt
  tests/ECMSetupVersionTest/old_version_file_exact/CMakeLists.txt
  tests/ECMSetupVersionTest/old_version_file_samemajor/CMakeLists.txt
  tests/ExecuteCoreModules/CMakeLists.txt
  tests/ExecuteKDEModules/CMakeLists.txt
  tests/FindModules/CMakeLists.txt
  tests/KDEFetchTranslations/CMakeLists.txt
  tests/KDEInstallDirsTest/relative_or_absolute/CMakeLists.txt
  tests/KDEInstallDirsTest/vars_in_sync/CMakeLists.txt
  tests/KDEPackageAppTemplatesTest/qml-plasmoid/CMakeLists.txt
  tests/UseFindModules/CMakeLists.txt

To: vonreth, dfaure, cullmann
Cc: aacid, chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns