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

[Christoph Cullmann]

cullmann closed this revision.
cullmann added a comment.


  For my 2 wanted changes, I created a new merge request
  https://invent.kde.org/frameworks/extra-cmake-modules/-/merge_requests/20

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure, broulik, davidedmundson
Cc: zzag, sitter, mwolff, ochurlaud, nalvarez, kossebau, aacid, davidedmundson, 
dhaumann, apol, ognarb, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
cblack, bencreasy, michaelh, ngraham, bruns

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

[Christoph Cullmann]

cullmann added a comment.


  In D24568#563653 , @cullmann wrote:
  
  > For the lambda issue, I think we can add:
  >
  >   # keep lambda formatting multi-line if not empty
  >   AllowShortLambdasOnASingleLine: Empty
  >
  >
  > see https://clang.llvm.org/docs/ClangFormatStyleOptions.html
  >
  > That fixes for me the issue with the collapsed lambda.
  >
  > For the comments, as said, we can remove
  >
  >   AlignTrailingComments: true
  >
  >
  > For the generic issue of stuff folded in one line, one can play with the 
ColumnLimit, but as said, that will still arbitrarily pack stuff as long as it 
fits the limit.
  >
  > Kai, could you try the two changes (for lambda and comments) by just 
temporarily changing that in the .clang-format file locally and rerunning make 
clang-format?
  
  
  Hi, would it be ok to add this two improvements?
  
  Beside that, has somebody tried the formatting again?

REPOSITORY
  R240 Extra CMake Modules

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

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

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

[Christoph Cullmann]

cullmann added a comment.


  Ping? Any update if somebody tried the proposed changes?

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure, broulik, davidedmundson
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann added a comment.


  In D24568#580338 , @kossebau wrote:
  
  > This has been missing the link from an rst file  in docs/, so the 
documentation generation picks up the file. Fixed with 
c4890d5c03ed79f0c87da861b6608bbd46c2162c 

  
  
  Thanks for adding that!
  
  Any feedback on the other issues people?

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure, broulik, davidedmundson
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Friedrich W. H. Kossebau]

kossebau added a comment.


  This has been missing the link from an rst file  in docs/, so the 
documentation generation picks up the file. Fixed with 
c4890d5c03ed79f0c87da861b6608bbd46c2162c 


REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure, broulik, davidedmundson
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann added a comment.


  Btw., I just tried e.g.
  
  https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format
  
  that one has exactly the same issues as our file and re-flows stuff into 
single lines (even lambdas).
  
  The only thing that often hinders that is the column limit of 100 there.
  
  That on the other side make as lot of code lot harder to read, too, like:
  
-
cc()->unregisterCompletionModel(KTextEditor::EditorPrivate::self()->wordCompletionModel());
 // would add additional items, we don't want that in tests
+cc()->unregisterCompletionModel(KTextEditor::EditorPrivate::self()
+->wordCompletionModel()); // 
would add additional items,
+  // 
we don't want that in tests

-
highlightMatchAttribute->dynamicAttribute(Attribute::ActivateMouseIn)->setBackground(searchColor);
-
highlightMatchAttribute->dynamicAttribute(Attribute::ActivateMouseIn)->setForeground(foregroundColor);
-
highlightMatchAttribute->dynamicAttribute(Attribute::ActivateCaretIn)->setBackground(searchColor);
-
highlightMatchAttribute->dynamicAttribute(Attribute::ActivateCaretIn)->setForeground(foregroundColor);
+highlightMatchAttribute->dynamicAttribute(Attribute::ActivateMouseIn)
+->setBackground(searchColor);
+highlightMatchAttribute->dynamicAttribute(Attribute::ActivateMouseIn)
+->setForeground(foregroundColor);
+highlightMatchAttribute->dynamicAttribute(Attribute::ActivateCaretIn)
+->setBackground(searchColor);
+highlightMatchAttribute->dynamicAttribute(Attribute::ActivateCaretIn)
+->setForeground(foregroundColor);
  
  (and that is just one of  places in KTextEditor that look like that 
afterwards).

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure, broulik, davidedmundson
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann added reviewers: broulik, davidedmundson.
cullmann added a comment.


  For the lambda issue, I think we can add:
  
# keep lambda formatting multi-line if not empty
AllowShortLambdasOnASingleLine: Empty
  
  see https://clang.llvm.org/docs/ClangFormatStyleOptions.html
  
  That fixes for me the issue with the collapsed lambda.
  
  For the comments, as said, we can remove
  
AlignTrailingComments: true
  
  For the generic issue of stuff folded in one line, one can play with the 
ColumnLimit, but as said, that will still arbitrarily pack stuff as long as it 
fits the limit.
  
  Kai, could you try the two changes (for lambda and comments) by just 
temporarily changing that in the .clang-format file locally and rerunning make 
clang-format?

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure, broulik, davidedmundson
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann reopened this revision.
cullmann added a comment.
This revision is now accepted and ready to land.


  Let's just reopen this and work on improving it.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann added a comment.


  In D24568#563092 , @davidedmundson 
wrote:
  
  > As an update on this from the Plasma POV.
  >
  > I added the macro to every repo and told every dev to do a final test 
before we commit the formatted results.
  >
  > I had some feedback and the result was that we can't proceed with in the 
current state [1].
  >
  > What's noteworthy is we were generally ok with the results from the first 
file we prepared in T11214 , so potentially 
we just need some settings tweaked. 
  >  I'll try and break that down into future diffs.
  >
  > 1.https://mail.kde.org/pipermail/plasma-devel/2019-November/106186.html
  
  
  For the comment alignment stuff, that is easy, we can just remove
  
  AlignTrailingComments: true
  
  For the other general "moving between different lines" stuff, I don't see any 
solution for this.
  
  You can arbitrary change the ColumnLimit, but as far as I know, it will just 
arbitrarily pack stuff together, like in the if conditions, if you have not 
e.g. comments in-between.
  
  But perhaps somebody else knows a proper switch for that.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Dominik Haumann]

dhaumann added a comment.


  You can force the current clang format to keep the multi-line if as follows:
  
if (roles.isEmpty() // comment
|| roles.contains(Notifications::UpdatedRole)
|| roles.contains(Notifications::ExpiredRole)
  
  The comment prevents clang to join the lines. I understand it's not what you 
want, though.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[David Edmundson]

davidedmundson added a comment.


  As an update on this from the Plasma POV.
  
  I added the macro to every repo and told every dev to do a final test before 
we commit the formatted results.
  
  I had some feedback and the result was that we can't proceed with in the 
current state [1].
  
  What's noteworthy is we were generally ok with the results from the first 
file we prepared in T11214 , so potentially 
we just need some settings tweaked. 
  I'll try and break that down into future diffs.
  
  1.https://mail.kde.org/pipermail/plasma-devel/2019-November/106186.html

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann added a comment.


  Volker Ok'd it that I used that on the co-maintained KSyntaxHighlighting 
framework, too.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann added a comment.


  Ok, I pushed this now.
  
  I will use that myself in KTextEditor and Kate instead of my old file and 
shell script.
  
  We can think about what to do with other frameworks now.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

This revision was automatically updated to reflect the committed changes.
cullmann marked an inline comment as done.
Closed by commit R240:235fcabf5018: Provide clang-format target with a KDE 
Frameworks style file (authored by cullmann).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24568?vs=68369=68380

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

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann added inline comments.

INLINE COMMENTS

> dfaure wrote in clang-format.cmake:75
> (b) looks good to me, and more importantly, I like the goal of being as close 
> as possible to the Qt coding style.

removed that setting, back to Webkit default of BreakBeforeTernaryOperators: 
true

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

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


  - keep default of BreakBeforeTernaryOperators = true

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24568?vs=68186=68369

BRANCH
  master

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

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> cullmann wrote in clang-format.cmake:75
> I have no strong opinion on that, we can remove that or keep it.

(b) looks good to me, and more importantly, I like the goal of being as close 
as possible to the Qt coding style.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann added inline comments.

INLINE COMMENTS

> zzag wrote in clang-format.cmake:75
> I've been always wondering how one should break long ternary operators when 
> writing KF code. There are several ways to do it
> 
> (a) w/o breaking (BreakBeforeTernaryOperators: false)
> 
>   const FooBar *foobar = someStupidCondition() ?
>   someSuperDuperBeatifulFunctionWithLongName() :
>   anotherSuperDuperBeatiflFunctionWithLongName();
> 
> (b) w/ breaking (BreakBeforeTernaryOperators: true)
> 
>   const FooBar *foobar = someStupidCondition()
>   ? someSuperDuperBeatifulFunctionWithLongName()
>   : anotherSuperDuperBeatiflFunctionWithLongName();
> 
> According to the _clang-format file from the qt5 super repo, Qt fellas prefer 
> (b) to break before ternary operators.
> 
> Do we really want to not break before ternary opeartors?

I have no strong opinion on that, we can remove that or keep it.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann added a comment.


  Has somebody tested the current state?
  
  I really would like to get this going as in my eyes this will finally put a 
working solution in place for the coding style enforcing on at least frameworks.
  
  One can later still improve stuff like "shall the gitlab stuff execute that 
on the patches" or "shall we let scripty run this once a month over all 
repositories that opt-in to this" to keep the style consistent.
  
  At company we just have the rule that one is free to run the company wide 
reformat target and commit that as some extra pure "reformat" commit if the 
coding style did deteriorate.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> dfaure wrote in clang-format.cmake:75
> typo: ternary

I've been always wondering how one should break long ternary operators when 
writing KF code. There are several ways to do it

(a) w/o breaking (BreakBeforeTernaryOperators: false)

  const FooBar *foobar = someStupidCondition() ?
  someSuperDuperBeatifulFunctionWithLongName() :
  anotherSuperDuperBeatiflFunctionWithLongName();

(b) w/ breaking (BreakBeforeTernaryOperators: true)

  const FooBar *foobar = someStupidCondition()
  ? someSuperDuperBeatifulFunctionWithLongName()
  : anotherSuperDuperBeatiflFunctionWithLongName();

According to the _clang-format file from the qt5 super repo, Qt fellas prefer 
(b) to break before ternary operators.

Do we really want to not break before ternary opeartors?

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann added a comment.


  Without the initializer change, the file works for me reasonable well, tried 
it again on KTextEditor.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann updated this revision to Diff 68186.
cullmann marked an inline comment as done.
cullmann added a comment.


  - avoid collapsing of constructor initializer lines

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24568?vs=68091=68186

BRANCH
  master

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

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

To: cullmann, #frameworks, dfaure
Cc: sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

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


  With
  
  BreakConstructorInitializers: BeforeColon
  
  you get collapsed stuff like;
  
  Range::Range(const KTextEditor::Cursor , const KTextEditor::Cursor c2, 
MotionType mt) : Range(c1.line(), c1.column(), c2.line(), c2.column(), mt)
  
  I think the behavior of the default of WebKit
  
  BCIS_BeforeComma (in configuration: BeforeComma) Break constructor 
initializers before the colon and commas, and align the commas with the colon.
  
  Constructor()
  
: initializer1()
, initializer2()
  
  is much more reasonable.
  
  One can play with
  
  ConstructorInitializerAllOnOneLineOrOnePerLine (bool)
  If the constructor initializers don’t fit on a line, put each initializer on 
its own line.
  
  true:
  SomeClass::Constructor()
  
  : (), (), 
(a) {
return 0;
  
  }
  
  false:
  SomeClass::Constructor()
  
  : (), (),
(a) {
return 0;
  
  }
  ConstructorInitializerIndentWidth (unsigned)
  The number of characters to use for indentation of constructor initializer 
lists as well as inheritance lists.

INLINE COMMENTS

> sitter wrote in KDEClangFormat.cmake:53
> I'm pretty sure you need to check the version the exectuable. When I use 6.0 
> I get ctors smushed into one line.

No, actually the reason for that is the added BreakConstructorInitializers: 
BeforeColon
I am not sure how to avoid that if we not go back to the old variant I had 
without that.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: sitter, mwolff, 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 KDE Frameworks style file

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> KDEClangFormat.cmake:53
> +# try to find clang-format in path
> +find_program(KDE_CLANG_FORMAT_EXECUTABLE clang-format)
> +

I'm pretty sure you need to check the version the exectuable. When I use 6.0 I 
get ctors smushed into one line.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: sitter, mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann added a comment.


  In D24568#548490 , @dfaure wrote:
  
  > Do we want these, found in 
https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format?
  >
  >   # We use template< without space.
  >   SpaceAfterTemplateKeyword: false
  >  
  >   # macros for which the opening brace stays attached.
  >   ForEachMacros:   [ foreach, Q_FOREACH, BOOST_FOREACH, forever, Q_FOREVER, 
QBENCHMARK, QBENCHMARK_ONCE ]
  >  
  >   # Break constructor initializers before the colon and after the commas.
  >   BreakConstructorInitializers: BeforeColon
  >
  
  
  added that
  
  > In general, I'm curious why we're not using qt5's clang-format file, with 
our only difference (braces for single-line statements) on top?
  
  I was more comfortable tweaking the file I created some years ago (and 
updated for all clang-format versions since then) that should be close to the 
kdelibs style than trying to take a look in detail at the other one, to be 
honest ;=)

INLINE COMMENTS

> dfaure wrote in KDEClangFormat.cmake:76
> I wonder if people compiling KF5 modules (and not necessarily planning on 
> contributing) need to be annoyed with such a warning. Maybe we could still 
> define the clang-format target and make it print an error?

Yeah, I think some message on running the target will be less annoying.

> dfaure wrote in clang-format.cmake:43
> I'm confused because here it says "true" will group via empty lines, while 
> https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format says `the SortInclude 
> feature of clang-format does not re-order includes separated by empty lines`. 
> Maybe different versions of clang-format? Or I misunderstand something?

Perhaps my comment is not understandable.
true means clang-format will sort stuff inside #include groups (aka #include 
lines without empty lines in-between)
Will alter the comment.

> dfaure wrote in clang-format.cmake:49
> https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format says 
> `PointerBindsToType: false`, what's the difference?

My file is for some recent clang, whereas PointerBindsToType seems to be 
ancient, see:

https://releases.llvm.org/3.4/tools/clang/docs/ClangFormatStyleOptions.html 
(there you still have that)

http://releases.llvm.org/9.0.0/tools/clang/docs/ClangFormatStyleOptions.html 
(here I can only find the variant I use)

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann updated this revision to Diff 68091.
cullmann marked 4 inline comments as done.
cullmann added a comment.


  - fix coding style issue, we don't want indented case labels
  - add initial docs
  - adjust style
  - just tell the user it will not work

REPOSITORY
  R240 Extra CMake Modules

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

BRANCH
  master

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

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

To: cullmann, #frameworks, dfaure
Cc: mwolff, 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 KDE Frameworks style file

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  I like the idea very much.

INLINE COMMENTS

> KDEClangFormat.cmake:76
> +else()
> +message(STATUS "Could not set up the clang-format target as the 
> clang-format executable is missing.")
> +endif()

I wonder if people compiling KF5 modules (and not necessarily planning on 
contributing) need to be annoyed with such a warning. Maybe we could still 
define the clang-format target and make it print an error?

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: mwolff, 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 KDE Frameworks style file

[David Faure]

dfaure added a comment.


  Do we want these, found in 
https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format?
  
# We use template< without space.
SpaceAfterTemplateKeyword: false

# macros for which the opening brace stays attached.
ForEachMacros:   [ foreach, Q_FOREACH, BOOST_FOREACH, forever, Q_FOREVER, 
QBENCHMARK, QBENCHMARK_ONCE ]

# Break constructor initializers before the colon and after the commas.
BreakConstructorInitializers: BeforeColon
  
  In general, I'm curious why we're not using qt5's clang-format file, with our 
only difference (braces for single-line statements) on top?

INLINE COMMENTS

> clang-format.cmake:42
> +
> +# sematics shall be grouped via empty lines
> +SortIncludes: true

Typo: semantics

> clang-format.cmake:43
> +# sematics shall be grouped via empty lines
> +SortIncludes: true
> +

I'm confused because here it says "true" will group via empty lines, while 
https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format says `the SortInclude 
feature of clang-format does not re-order includes separated by empty lines`. 
Maybe different versions of clang-format? Or I misunderstand something?

> clang-format.cmake:49
> +# CrlInstruction *a;
> +PointerAlignment: Right
> +

https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format says `PointerBindsToType: 
false`, what's the difference?

> clang-format.cmake:75
> +
> +# don't break tenary ops
> +BreakBeforeTernaryOperators: false

typo: ternary

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: mwolff, 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 KDE Frameworks style file

[Christoph Cullmann]

cullmann added a reviewer: dfaure.
cullmann added a comment.


  Perhaps David could give feedback if the file actually captures the intend to 
do proper KDE Frameworks/libs like formatting.
  I had a mistake with the indented case statements, that should be fixed.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: mwolff, 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 KDE Frameworks style file

[Milian Wolff]

mwolff added a comment.


  In D24568#545942 , @cullmann wrote:
  
  > 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.
  
  
  For our projects at work where we have the liberty to dictate the coding 
style, we also use clang format. To do that properly, we have a pre-commit 
check locally and then a gerrit bot similar to the Qt coding style bot which 
checks the formatting introduced by a patch. This way, one can be sure that the 
style doesn't deteriorate over time.
  
  Some links on that matter:
  
  - https://github.com/nghttp2/nghttp2/blob/master/pre-commit is what I use to 
check my commits, there are probably other, better hooks available somewhere, 
but this one has suited me well so far
  - when the hook complains, I run `git clang-format` which is part of the 
clang package that also ships clang-format on ArchLinux
  
  I guess the commit hook can also be used for a server check, but I'm not sure 
how this is done with gerrit for us, or how one would do it with 
phabricator/gitlab for KDE.
  
  Anyhow: big +1 form my side for using clang-format for all of KDE 
(eventually) and KDE frameworks in the near future as a starting point.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks
Cc: mwolff, 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 KDE Frameworks style file

[Olivier Churlaud]

ochurlaud added a comment.


  In D24568#546289 , @kossebau wrote:
  
  > 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).
  >
  > @nalvarez @ochurlaud  Do you remember why this page was not moved?
  
  
  This must have been a mistake. Please make sure it's findable on community.ko!

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 KDE Frameworks style file

[Christoph Cullmann]

cullmann retitled this revision from "Provide clang-format target with a common 
KDE style file" to "Provide clang-format target with a KDE Frameworks style 
file".

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