D24568: Provide clang-format target with a common KDE style file

[Friedrich W. H. Kossebau]

kossebau added subscribers: nalvarez, ochurlaud.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks
Cc: ochurlaud, nalvarez, kossebau, aacid, davidedmundson, dhaumann, apol, 
ognarb, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Friedrich W. H. Kossebau]

kossebau added a comment.


  There is also https://techbase.kde.org/Policies/Frameworks_Coding_Style which 
though missed the move from techbase to community, other than the other 
policies.
  
  I suspect that page should be moved over now as well, to become the real KF 
coding style page (so that "kdelibs" named one is no longer needed).

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks
Cc: kossebau, aacid, davidedmundson, dhaumann, apol, ognarb, 
kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, 
ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Christoph Cullmann]

cullmann added a comment.


  I hope this file implements 
https://community.kde.org/Policies/Kdelibs_Coding_Style
  
  And that is the style all frameworks got astyled with on initial import.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks
Cc: aacid, davidedmundson, dhaumann, apol, ognarb, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Albert Astals Cid]

aacid added a comment.


  If the KDE Frameworks developers have agreed to a common style, then yes, 
naming it KDE Frameworks style makes sense :)

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks
Cc: aacid, davidedmundson, dhaumann, apol, ognarb, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Christoph Cullmann]

cullmann added a comment.


  In D24568#546227 , @aacid wrote:
  
  > > common KDE style file
  >
  > There's no such thing as a common KDE style
  
  
  Shall we name it kdelibs coding style?
  But actually the idea is to have one, as opt in, for the repos that want that.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks
Cc: aacid, davidedmundson, dhaumann, apol, ognarb, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Albert Astals Cid]

aacid added a comment.


  > common KDE style file
  
  There's no such thing as a common KDE style

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks
Cc: aacid, davidedmundson, dhaumann, apol, ognarb, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Christoph Cullmann]

cullmann added a comment.


  If there are more deviations from the kdelibs/frameworks coding style, please 
tell me.
  
  Otherwise, I am happy with the output of this file for KTextEditor & Kate.
  
  For the .clang-format instantiation question: I think it makes sense to 
instantiate it on initial cmake running, as otherwise the tooling users use for 
clang-format/clangd will not pick up the style.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks
Cc: davidedmundson, dhaumann, apol, ognarb, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Christoph Cullmann]

cullmann added a reviewer: Frameworks.
cullmann marked an inline comment as done.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks
Cc: davidedmundson, dhaumann, apol, ognarb, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Christoph Cullmann]

cullmann updated this revision to Diff 67797.
cullmann added a comment.


  - add initial docs

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24568?vs=67796=67797

BRANCH
  master

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

AFFECTED FILES
  kde-modules/KDEClangFormat.cmake
  kde-modules/clang-format.cmake

To: cullmann
Cc: davidedmundson, dhaumann, apol, ognarb, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Christoph Cullmann]

cullmann marked an inline comment as done.
cullmann added inline comments.

INLINE COMMENTS

> ognarb wrote in KDEClangFormat.cmake:11
> need doc

Added some initial docs

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann
Cc: davidedmundson, dhaumann, apol, ognarb, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Christoph Cullmann]

cullmann updated this revision to Diff 67796.
cullmann added a comment.


  - fix coding style issue, we don't want indented case labels

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24568?vs=67739=67796

BRANCH
  master

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

AFFECTED FILES
  kde-modules/KDEClangFormat.cmake
  kde-modules/clang-format.cmake

To: cullmann
Cc: davidedmundson, dhaumann, apol, ognarb, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Christoph Cullmann]

cullmann added inline comments.

INLINE COMMENTS

> davidedmundson wrote in KDEClangFormat.cmake:48
> it's a bit weird to put new files in the source directory when running cmake.
> 
> Copying when running the target clang-format would probably be ok as there 
> you're already doing an explicit operation which changes the source.

If we don't do this per default, user clang-format tooling like via LSP in 
Atom/Vscode/Kate will not work properly before a manual action.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann
Cc: davidedmundson, dhaumann, apol, ognarb, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> KDEClangFormat.cmake:48
> +if(KDE_CLANG_FORMAT_EXECUTABLE)
> +configure_file(${CMAKE_CURRENT_LIST_DIR}/clang-format.cmake 
> ${CMAKE_CURRENT_SOURCE_DIR}/.clang-format @ONLY)
> +endif()

it's a bit weird to put new files in the source directory when running cmake.

Copying when running the target clang-format would probably be ok as there 
you're already doing an explicit operation which changes the source.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann
Cc: davidedmundson, dhaumann, apol, ognarb, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Christoph Cullmann]

cullmann added a comment.


  In D24568#545736 , @apol wrote:
  
  > I'm not sure how this works, but would it be possible to have a target that 
only works on a patch? You usually want to make sure what you modified didn't 
diverge from the code.
  
  
  I think there is some hack around that:
  http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting
  
  But actually, if your sources are already clang-formatted, you just need to 
run the clang-format target once before you commit, the your new code will be 
the only thing altered.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann
Cc: dhaumann, apol, ognarb, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Dominik Haumann]

dhaumann added a comment.


  I'm all for it. This would unify how we can reformat any KDE module, which is 
very much desirable.
  
  Being able to just reformat a patch sounds interesting, too, but can be added 
later still.
  
  Don't let the perfect be the enemy of the good...
  
  The example usage should go to the location that Carl pointed out.
  
  +1, can we have another iteration?

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann
Cc: dhaumann, apol, ognarb, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a common KDE style file

[Carl Schwan]

ognarb added a comment.


  There is probably a way to run clang-format only on a patch. See 
http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting
  
  But should we not prefer running clang format one time, instead of having 
only some part of the code auto-formatted?

REPOSITORY
  R240 Extra CMake Modules

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

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

D24568: Provide clang-format target with a common KDE style file

[Aleix Pol Gonzalez]

apol added a comment.


  I'm not sure how this works, but would it be possible to have a target that 
only works on a patch? You usually want to make sure what you modified didn't 
diverge from the code.

REPOSITORY
  R240 Extra CMake Modules

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

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

D24568: Provide clang-format target with a common KDE style file

[Carl Schwan]

ognarb added inline comments.

INLINE COMMENTS

> KDEClangFormat.cmake:11
> +#
> +# ::
> +#

need doc

REPOSITORY
  R240 Extra CMake Modules

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

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

D24568: Provide clang-format target with a common KDE style file

[Christoph Cullmann]

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

REVISION SUMMARY
  Provides a clang-format target if wanted
  
  Example usage:
  
  include(KDEClangFormat)
  
  add clang-format target for all our real source files
  =
  
  file(GLOB_RECURSE ALL_CLANG_FORMAT_SOURCE_FILES autotests/src/*.cpp 
autotests/src/*.h src/*.cpp src/*.h templates/*.cpp templates/*.h)
  kde_clang_format(${ALL_CLANG_FORMAT_SOURCE_FILES})

TEST PLAN
  Tried that above usage thingy in KTextEditor

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

AFFECTED FILES
  kde-modules/KDEClangFormat.cmake
  kde-modules/clang-format.cmake

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