Clang Format Update

Kai Uwe Broulik kde at privat.broulik.de
Thu Nov 14 17:44:01 GMT 2019


Hi,

I've just ran make clang-format (thanks a lot for that tooling!) over 
plasma-workspace and checked my libnotificationmanager code which is 
pretty new, was only written a few months ago, and as such should be a 
good representation of modern KDE Frameworks-style code. The results 
were not very promising with that format file currently in ECM.

While there are changes that I agree with or don't really mind, like 
removing an empty line between beginning of namespace and begging of a 
class, or moving one line case returns into a separate line, there were 
a lot of changes that reduced legibility of code significantly, by 
moving statements wrapped into multiple lines into one giant line, e.g.

-    template<typename T> bool updateField(const T &newValue,
-                                          T &target,
-                                          void (Job::*changeSignal)())
+    template<typename T> bool updateField(const T &newValue, T &target, 
void (Job::*changeSignal)())

or

-            if (roles.isEmpty()
-                    || roles.contains(Notifications::UpdatedRole)
-                    || roles.contains(Notifications::ExpiredRole)
-                    || roles.contains(Notifications::JobStateRole)
-                    || roles.contains(Notifications::PercentageRole)) {
+            if (roles.isEmpty() || 
roles.contains(Notifications::UpdatedRole) || 
roles.contains(Notifications::ExpiredRole) || 
roles.contains(Notifications::JobStateRole) || role
s.contains(Notifications::PercentageRole)) {

Furthermore, it randomly aligned doxygen comments in enums in columns 
but not all of them on the same column but like five of them in one 
column and then the next enum value was too long so it indented it further

Foo,    ///< Foo
FooBar, ///< FooBar
FooBarLong,    ///< FooBarLong
FooBarMore,    ///< FooBarMore

It also had the tendency to move braces to the adjacent line, instead of

SomeCallWith({
     A,
     lot,
     of,
     assignments
});

it would do

SomeCallWith({A,
               lot,
               of,
               assignments});

which also makes the code annoying to refactor as you would touch the 
previous line when adding a new item as you have to move the "})" around.

However, biggest deal breaker and completely unacceptable was it moving 
single statement lambdas into the same line:

-    auto it = std::find_if(notifications.constBegin(), 
notifications.constEnd(), [id](const Notification &item) {
-        return item.id() == id;
-    });
+    auto it = std::find_if(notifications.constBegin(), 
notifications.constEnd(), [id](const Notification &item) { return 
item.id() == id; });

In total for libnotificationmanager:
  37 files changed, 407 insertions(+), 554 deletions(-)

Out of those changes it feels like 3/4 were flat out wrong or 
counterproductive.

Cheers
Kai Uwe


More information about the Plasma-devel mailing list