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