Differential D20572

RFC: Add .clang-format
AbandonedPublic
Actions

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

Details

Reviewers

Group Reviewers

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

Harbormaster completed remote builds in B10896: Diff 56294.Apr 15 2019, 12:37 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";
        });
}

}

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?

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.

Sneaky update.

Harbormaster completed remote builds in B10897: Diff 56296.Apr 15 2019, 1:36 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.

In D20572#450586, @davidedmundson wrote:

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.

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.

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.Nov 12 2019, 12:41 PM

zzag abandoned this revision.Nov 12 2019, 12:43 PM

Revision Contents
Changeset List

			Path	Packages
A	M		.clang-format (71 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	56294	e3d1ddd		Apr 15 2019, 12:37 PM	★	★
Diff 2	56296	e3d1ddd	Sneaky update.	Apr 15 2019, 1:36 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
16db9ba5e742	5117cc9c2a25	e3d1ddd2bf0a	Vlad Zagorodniy	RFC: Add .clang-format (Show More…)	Apr 15 2019, 10:04 AM

Diff 56296

View Options

RFC: Add .clang-formatAbandonedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 56296

.clang-format

RFC: Add .clang-format
AbandonedPublic
Actions

Revision Contents
Changeset List