RFC: Add .clang-format
AbandonedPublic

Authored by zzag on Apr 15 2019, 12:37 PM.

Details

Reviewers
davidedmundson
Group Reviewers
KWin
Summary

Different developers prefer different coding styles, e.g. some people
align pointers to the left and some to the right, etc. Unfortunately,
there are changes that don't follow the kdelibs/Frameworks coding style.
Reviewing such changes gets a bit tedious for both code reviewer and
patch author. The worst part is that some coding style incompliance
issues go in unnoticeable, which is okay, we're humans after all...

Luckily, LLVM project developed a tool called clang-format. As its name
suggests, clang-format is a tool to format C/C++ code. In order to use
it, we have to provide our own .clang-format file where different style
options are specified (e.g. pointer alignment, etc). As a developer
you need to enable auto-formatting in your text editor (or just setup
a pre-commit hook to run clang-format).

Though clang-format isn't perfect. The biggest issue with it right now
is that there is no any option to disable formatting of SIGNALs and
SLOTs (using clang-format off and clang-format on is unacceptable).
This is probably not a big issue because we have to replace majority of
string-based connections with functor-based connections anyway.

Diff Detail

Repository
R108 KWin
Branch
add-clang-format
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 10897
Build 10915: arc lint + arc unit
zzag created this revision.Apr 15 2019, 12:37 PM
Restricted Application added a project: KWin. · View Herald TranscriptApr 15 2019, 12:37 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Apr 15 2019, 12:37 PM
zzag added a comment.Apr 15 2019, 12:39 PM

Before:

#include "raw.h"

#include "abstract_client.h"
#include <algorithm>
#include <QComboBox>
#include <QWidget>
#include <QAbstractEventDispatcher>
#include <KWayland/Server/xdgshell_interface.h>
#include <KWayland/Client/xdgshell.h>
#include "cursor.h"

// Another block.
#include "workspace.h"

namespace KWin {

static inline void foo() { qDebug( )<< "Foo!";}
static inline void foo() {
    qDebug( )<< "Foo!";}

class Client : public QObject {
    Q_OBJECT

    public:

        explicit Client(QObject* parent=nullptr);
        ~Client() override;

        Client(Client&&other);
        Client(Client&& other);
        Client(Client& &other);
        Client(const Client& other);

        Group* group( ) const;

        int id() const { return m_id; }
        int id() const {
            return m_id;
        }

    protected:
        void crash();
    private:
        void leakMemory();
        void corruptData();
    private:
        int m_id;
};

void Client::crash ()
{
    int* x = nullptr;
    *x=42;
}

void Client::leakMemory() {
    int x = 42;

    switch(x)
    {
        case 0:
        case 1: new int; break;
        case 2: new int[2]; break;
        case 42: new int[42]; break;
        default:
            crash();
                break;
    }



    if(2 + 2 == 5)
    {
        foo ();
    }
    else
    {
        crash();
    }

    connect(this, &Client::foobared,
            this, &QObject::deleteLater);
    disconnect(this, &Client::foobared,
               this, &QObject::deleteLater);

    const int expr = 1 + 2 +
        3;
    const int expr = 1 + 2
        + 3;
    const int expr =
        1 + 2 + 3;

    if (2 + 2 == 4)
    crash();

    connect(this, &Client::foobared, this, [this] {
                qDebug() << "foobared";
            });
    connect(this, &Client::foobared, this,
        [this] {
            qDebug() << "foobared";
        }
    );
}

}

After running clang-format:

#include "raw.h"

#include "abstract_client.h"
#include "cursor.h"
#include <KWayland/Client/xdgshell.h>
#include <KWayland/Server/xdgshell_interface.h>
#include <QAbstractEventDispatcher>
#include <QComboBox>
#include <QWidget>
#include <algorithm>

// Another block.
#include "workspace.h"

namespace KWin
{
static inline void foo() { qDebug() << "Foo!"; }
static inline void foo()
{
    qDebug() << "Foo!";
}

class Client : public QObject
{
    Q_OBJECT

public:
    explicit Client(QObject *parent = nullptr);
    ~Client() override;

    Client(Client &&other);
    Client(Client &&other);
    Client(Client &&other);
    Client(const Client &other);

    Group *group() const;

    int id() const { return m_id; }
    int id() const
    {
        return m_id;
    }

protected:
    void crash();

private:
    void leakMemory();
    void corruptData();

private:
    int m_id;
};

void Client::crash()
{
    int *x = nullptr;
    *x = 42;
}

void Client::leakMemory()
{
    int x = 42;

    switch (x) {
    case 0:
    case 1:
        new int;
        break;
    case 2:
        new int[2];
        break;
    case 42:
        new int[42];
        break;
    default:
        crash();
        break;
    }

    if (2 + 2 == 5) {
        foo();
    } else {
        crash();
    }

    connect(this, &Client::foobared,
        this, &QObject::deleteLater);
    disconnect(this, &Client::foobared,
        this, &QObject::deleteLater);

    const int expr = 1 + 2 + 3;
    const int expr = 1 + 2
        + 3;
    const int expr =
        1 + 2 + 3;

    if (2 + 2 == 4)
        crash();

    connect(this, &Client::foobared, this, [this] {
        qDebug() << "foobared";
    });
    connect(this, &Client::foobared, this,
        [this] {
            qDebug() << "foobared";
        });
}

}
apol added a subscriber: apol.Apr 15 2019, 12:57 PM

We could give it a go, how do you test it?
Do you suggest having it in git hooks? Can we use it only on the places that change?

zzag added a comment.Apr 15 2019, 1:11 PM
In D20572#450547, @apol wrote:

We could give it a go, how do you test it?

One way would be

clang-format -style=file <filename>

Though most text editors have clang-format integration, which makes things much easier. For example, Qt Creator has clang-format support out-of-box, no need to install plugins, etc.

Do you suggest having it in git hooks?

Well, this is an option.

Can we use it only on the places that change?

Might be worth to look into http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting.

zzag updated this revision to Diff 56296.Apr 15 2019, 1:36 PM

Sneaky update.

davidedmundson added a subscriber: davidedmundson.EditedApr 15 2019, 1:44 PM

Reviewing such changes gets a bit tedious for both code reviewer and patch author.

I very much agree with this!!


I think this is bigger than kwin and something worth taking to kde-devel, plasma-devel.

Applying to all files in the project is something that IMHO makes sense, and something I would readily approve in plasma-desktop/plasma-workspace.

apol added a comment.Apr 15 2019, 2:15 PM

Reviewing such changes gets a bit tedious for both code reviewer and patch author.

I think this is bigger than kwin and something worth taking to kde-devel, plasma-devel.

Applying to all files in the project is something that IMHO makes sense, and something I would readily approve in plasma-desktop/plasma-workspace.

coding style changes easily pollute the history though, with not much value. Having patches land in the right coding style is a clean way to move forward.

zzag added a comment.Apr 15 2019, 2:29 PM
In D20572#450602, @apol wrote:

coding style changes easily pollute the history though, with not much value.

Not really. That way we would have a mix of different coding styles...

It's worth to mention that if you change parts of code with wrong coding style, then your diffs will have a bunch of unrelated changes.

abetts added a subscriber: abetts.Apr 15 2019, 2:52 PM
abetts removed a subscriber: abetts.
davidedmundson requested changes to this revision.Tue, Nov 12, 12:41 PM

We're moving forwards with this idea, but we don't need this specific patch. See relevant plasma-devel emails.
Inclusion of the relevant ECM macro is added already.

This revision now requires changes to proceed.Tue, Nov 12, 12:41 PM
zzag abandoned this revision.Tue, Nov 12, 12:43 PM