Choosing a project-wide indentation style.

[sledgehammer999]

Some of you have already noticed that qbt code doesn't follow the same indentation style for all of its code.
I started maintaining the project almost a year ago. I tried to follow one specific indentation style for new code. Here is an example of it.

Opening curly braces should be on the same level as the function or statement:

int myFunction(bool a) {
// do some stuff
}

Else or else if shouldn't follow a closing curly brace but instead be on its own line

if (condition) {
 // do something
// do something too
}
else {
// blah
// blah
}

Also single statement(?) ifs shouldn't have curly braces

if (condition)
   return 0;
else
   return 155;

or

if (condition) break;

There is one exception on the above rule. Readability. There are cases where we have either too many nested if/else blocks or too many branches. In this case single statement ifs should use braces too.

Contributor @glassez believes that having the opening curly braces on their own line is better. And he also noted the lack of guidelines. By his view the code would look like this:

int myFunction(bool a)
{
  //do something
}

Of course I like my style better. I feel like the other style wastes whitespace. And I feel QtCreator does a great job drawing my style.

HOWEVER, I don't want to force my style down your throats. So I would like your comments and votes on which style to use. Or even suggestion of some other style. Or a suggestion for some other detail. (eg how to style if/else blocks).

This issue will have 2 stages:

1. Decide on a style. Then write the guidelines in a file and upload it to the repo. Also create an entry in the wiki.
2. After we have decided a style, ALL qbt source files should be converted to that style. I believe there are a lot of tools out there that can automate this. (and probably qtcreator has this feature already). This stage could be postponed until all pending pull requests have been merged. -it will probably brake them-

[sorokin]

I think this is more of stylistic issue than a technical one. So I think it is not very important. But as this issue is created let the flame begins!

Contributor @glassez believes that having the opening curly braces on their own line is better. And he also noted the lack of guidelines. By his view the code would look like this:

+1

[sledgehammer999]

Tagging recent contributors for comments: @Gelmir @glassez @alfrix @botanegg @john-peterson @sorokin @pmzqla @paolo-sz @BrunoReX @alderz @DoumanAsh

(sorry if I forgot someone)

[sledgehammer999]

@sorokin Let's be clear. You are supporting opening curly braces on their lines, right?

[sorokin]

Let's be clear. You are supporting opening curly braces on their lines, right?

@sledgehammer999 yes

void f()
{
}

[sledgehammer999]

Some tips for anyone wanting to give example on indentation using code. Here is how have a code block on github comments:
Start with 3 ` (backtick?) in a row. Then follow with the phrase "c++" on the same row after the 3 backticks without any space. Then hit enter for a newline. Put your code. Then to close put in a new line 3 backticks.

[sorokin]

Just a random thought. Could we discuss using 4 spaces for indentation? :-)

[sledgehammer999]

Sure. Anything related to indentation is welcome here.
PS: What are the current settings of qbt source files regarding that? I know that a lot of people fuss over spaces and tabs but I don't understand the merits of each side.

[sorokin]

What are the current settings of qbt source files regarding that?

spaces, 2.

I don't understand the merits of each side.

There aren't any. Just a matter of style. :-)

[sledgehammer999]

So @sorokin makes a suggestion of having 4 spaces as indentation. This is up for a vote too.

[DoumanAsh]

I agree with @sorokin proposal for 4 spaces.
Regarding main idea. I disagree with @glassez proposal.
Although i do not contribute to C++ code of qB(yet...) i have two projects on my work with two difference styles which are mentioned in thread. And for me it doesn't really look much of difference.
I prefer compact code so i don't see much of point in spending whole line just for curly brace

for IFs with signle statement i believe there is no need for braces and actually i think that statement in IF operator should not be placed on next line. I don't really see much of point in that if you can just align your code so it could be look like a more compact switch()
i.e.

if (condition) return 0;
else return 155;

Take for example this piece of code:

if (alternative_speeds)
    down_limit = pref->getAltGlobalDownloadLimit();
  else
    down_limit = pref->getGlobalDownloadLimit();
  if (down_limit <= 0) {
    // Download limit disabled
    setDownloadRateLimit(-1);
  } else {
    // Enabled
    setDownloadRateLimit(down_limit*1024);
  }

if (alternative_speeds) down_limit = pref->getAltGlobalDownloadLimit();
else down_limit = pref->getGlobalDownloadLimit();

if (down_limit <= 0)  setDownloadRateLimit(-1); // Download limit disabled
else setDownloadRateLimit(down_limit*1024); // Enabled

For me it looks the same. The original way is a little too big for me and i would prefer to have the second.
Anyway it's only me opinion.

Also i believe that there should be space after each coma. It should be the same for everyone but still it would be nice to have such rule :)

Last i totally agree that there should be some sorta design rules which should be followed by everyone. It will make code look better

[botanegg]

Hi.
I like standard CLion formatting style (e.g @DoumanAsh suggestion about compact 'if')

botanegg@7d82e7f
I formatted 'src/misc.cpp' (set indentation to 2 to get accurate diff), but i vote for 4 spaces 👍
botanegg@e5714e7

[pmzqla]

Regarding the indentation, I'm used to tabs, so I agree with the bump in the number of spaces from 2 to 4.

As for the braces, I like the style of the Linux kernel (Chapter 3: Placing Braces and Spaces), but I'm pretty much open to anything as long as there are some defined guidelines.

[sledgehammer999]

I like standard CLion formatting style

If you would like to propose another style please give examples.

And since I read linux kernel's style:
Let's split the decision on braces. Braces on function blocks and braces on other blocks (if, switch, while ... ).
Vote on these two.

[botanegg]

https://gist.github.com/botanegg/73cb6871628bd460b525

short something like java

[sledgehammer999]

So basically @botanegg wants opening braces on the same line for both functions and if/switch.
However he also wants the else if to be on the same line as the previous closing brace.
I have to disagree with the last one. On QtCreator, if you click "minus" sign on the left side of the editor to collapse the "if" block the "else if" that is on the same line as the closing brace will also be hidden. In this case you will not be able to see which tests are being performed.

[pmzqla]

@sledgehammer999 that's weird, it doesn't do that to me (Qt Creator 3.2.1).
} else { is also the style suggested in the Qt Creator Coding Rules, maybe you have an old version of the IDE.

[glassez]

Hello everyone!
As I suggested @sledgehammer999 to discuss the issue, make some remarks on your comments...
In my opinion, the qBittorrent code looks awful. At first he just scared me. And not only in its formatting... It just does not acceptable in a community developed project. So my main reason for the approval of certain rules of code styling (and yet some things) - is improving its readability and ease of his support.
Many of you in his comments just write "I like the way", others - "but I like it that way", without specifying the reasons.

I feel like the other style wastes whitespace.

...

I prefer compact code so i don't see much of point in spending whole line just for curly brace.

From increasing the number of lines in the source program will not increase. Sources formatted for people, not for the compiler (we can write all in one line for compiler).
I did not insist on all my preferences, I just want us to be in the discussion based on objective reasons.

The following are the issues that is necessary to solve.

1. File encoding and line ending. It must be the same in all the text files of the project. I prefer UTF-8 and Unix-like line ending (LF).
2. Type and width of indent. I prefer 4 spaces.
3. Formatting rules (curly braces, braces, spaces and so on).
4. Optional. Avoiding long procedures in the code.

[pmzqla]

Many of you in his comments just write "I like the way", others - "but I like it that way", without specifying the reasons.

I didn't argument my choices mostly because there are no arguments, I support what I'm used to with no specific reasons. As long as the code is readable, it's fine for me.

In general, I prefer a compact style that doesn't affect the readability of the code. That means I'm usually against using one line for if statements, unless the statement is really simple. However in that case I'd use the ternary operator directly, so in my opinion anything that is hardly readable in a ternary form needs to be written on multiple lines.

I support the idea of saving some lines as the chances of having entire blocks visible without scrolling are higher, so I'd skip braces if not necessary and wouldn't put them on separate lines.

Another thing I'd like to bring up is line wrapping. Even though most of the text editors out there can wrap long lines automatically, I personally prefer wrapping the code manually at a given column.

That said, the Qt Creator rules I linked in my previous comment are pretty close to how I'd write the code.

[chrishirst]

Some of you have already noticed that qbt code doesn't follow the same indentation style for all of its code.

Oh man! It is was only the 'indentation style' I'd be a really happy bunny. :)

[Gelmir]

recent contributors
@Gelmir

easy there buddy =)

Talking serious there are more points worth considering:

1. How to perform line breaks for long lines (usually method declarations/definitions and string assignments)? After operator or before operator?
   e.g.

   a += "b" +
     "c" +
     "d";

   or

   a += "b"
     + "c"
     + "d";

2. Are spaces mandatory around operators? a = b + c or a=b+c

3. Where to put reference/pointer sign in declarations and/or method arguments? int* a; or int *a;

4. private/public/protected are indented or not?

5. Level of tolerance for possibly retarded solutions? see https://github.com/qbittorrent/qBittorrent/pull/744/files#diff-30cc9c22fe6f1a06de992b6347a85f85R457

6. preprocessor commands follow indentation of previous line or go at line start?

7. Tolerance for inline comments? if (whatever) { // Some comment

8. Comments go after the line they are referring to or after?

9. Is ternary operation condition ? if true : else considered ok?

10. goto anyone? =)

11. Are method definitions allowed in header files? If yes then at what threshold?

12. Should we use built-in qt int types instead of c ones? quint8 instead of unsigned char

P.S. My personal favourite: explicit void in function declarations/definitions, e.g. int noarguments(void) { return 0; }
P.P.S. Might we worth reading: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html

[glassez]

QtCreator Formatting Rules look like alternative between my own preferences and yours (and current qBittorrent formatting). Some rules from Linux kernel formatting style must be added to them:

1. Use a space after (most) keywords. The notable exceptions are sizeof, typeof, alignof, and attribute, which look somewhat like functions (and are usually used with parentheses).
2. Do not add spaces around (inside) parenthesized expressions.
3. Use one space around (on each side of) most binary and ternary operators, such as any of these:
   = + - < > * / % | & ^ <= >= == != ? :
   but no space after unary operators:
   & * + - ~ ! sizeof typeof alignof attribute defined
   no space before the postfix increment & decrement unary operators:
   ++ --
   no space after the prefix increment & decrement unary operators:
   ++ --
   and no space around the '.' and "->" structure member operators.

[glassez]

@Gelmir Way of 1000 miles begins with the first step.
First we need to define the basic things. And then to decide how deep need rules.

My personal favourite: explicit void in function declarations/definitions

@Gelmir Apparently, you programs a long time.

[DoumanAsh]

@Gelmir

1. second
2. yes
3. second
4. yes
5. no idea....
6. line start
7. yes
8. before :)
9. ok
10. do you really need? :)
11. dunno...
12. stdint.h better....

[glassez]

private/public/protected are indented or not?

class Foo
{
    public:
        Foo();
};

class Bar
{
public:
    Bar();
};

Did you mean it?
Of course the second! Extra indents before access specifiers are excess.
As well as before case labels:

switch(val) {
case 1:
    // do something...
    break;
case 2:
    // do something...
    break;
default:
    // do something...
}

[alderz]

Personally I prefer 4 spaces for indentation, I feel it looks clearer and helps differentiate the blocks.

As for the style of the curly braces, I am used to do

int foo()
{

for functions and

if (x > 0) {
} else {
}

for conditionals and other blocks. Although, it is not really that important as long as there is consistency, so I am good with any way.

[glassez]

Are method definitions allowed in header files? If yes then at what threshold?

I think - yes. But only simple getters/setters or constructors/destructors with empty body.

class Foo : public Bar
{
public:
    Foo(int arg1, int arg2) : Bar(arg1), val_(arg2) {}
    ~Foo() {}

    void setValue(int val) { val_ = val; }
    int getValue() const { return val_; }

    void moreComplexFunc(int param); // defined in .cpp file

private:
    int val_;
};

[chrishirst]

FFS!!!!!!!!!!

Stop acting like spoilt children! NOBODY gives a toss who prefers what or why.

The decision is the maintainers to make and EVERBODY ELSE falls in line, HE is project leader and therefore THE authority on the semantics. This whole problem STARTED because of every developer doing what they prefer, so now it is time to make qbittorrent become a cohesive project rather than a dog's breakfast of disparate coding styles and 'conventions'.

[sledgehammer999]

@chrishirst

Why are you acting like this? I specifically invited people to propose styles/standards and then vote on that. It's true that until now there wasn't a constistent style followed by everyone that resulted in the current qbt stylistic mess. That's why now we will define a standard, create a document and have everyone follow it.

@ everyone
Tomorrow I am going to summarize everything in my first post, and record each commenter's preference/vote there. After everything is settled down I am going to leave it open for 1 week.

[glassez]

One more remark. I hope it is clear that if we use any files from other projects (or embed source libraries), you will not need to change their formatting (only encoding and line endings if necessary). Or I am not right?..

[glassez]

That has nothing to do with the issue at hand, why would you post that here? If you don't think it's worth exploring, coming here and posting angry rants is not the right way to go about it.
Just abstain.

+1

[chrishirst]

Because I'm trying to get you to see that there are FAR MORE pressing concerns than how many spaces to indent the code with.

[glassez]

@chrishirst How much do you think the time spent on each of us is here to express his opinion?

[sledgehammer999]

Yeah the rules are pretty much decided IMO. I just need to make them official.
Anyway, most of the devs here, they are not only "bickering" but they also push pull requests at the same time. So you cannot say that development has stalled due to this.
(and since this is a community project driven by voluntarily coding, each dev codes whatever he likes. I cannot tell some to go to develop feature X).

[sledgehammer999]

I created the wiki page and I am going to make commit too with the rules.
Thank you for the input.
Wiki page: https://github.com/qbittorrent/qBittorrent/wiki/Coding-style

[Gelmir]

Would be nice to have coding style template for Qt Creator.

[sorokin]

Are reformatting patches acceptable?

[sledgehammer999]

Would be nice to have coding style template for Qt Creator.

+1

Are reformatting patches acceptable?

Of course. Be sure to mention this bug number in the commit message for tracing back.

[pmzqla]

I have a question. The major difference between now and then is the number of spaces used to indent the code. How do we deal with this change?

Should new requests include at least two commits, one to indent the files that are going to be changed and the others with the actual changes, or just one commit?
Should we just change the style for the line touched? (IMO this will make the code ugly)

In the first case, even if "git blame" and "git diff" are able to ignore whitespace changes, I think that splitting the commits will make things more clear.

[sledgehammer999]

Should new requests include at least two commits, one to indent the files that are going to be changed and the others with the actual changes, or just one commit?

I was thinking of doing the change in spaces just before releasing v3.2.0. However, your way makes sense too.
First commit would be "Change indentation to 4 spaces. Issue #2192." That would include ALL files touched by the second commit.
Second commit would actually be the bug fixing one.

(I opened #2192 to track this)

[Gelmir]

@sledgehammer999 is missing indentation in https://github.com/qbittorrent/qBittorrent/blob/master/CODING_GUIDELINES.md at 1.(a, b, c) intentional?
e.g.

int myFunction(int a)
{
//code
}

[sledgehammer999]

Do you mean the un-indented comment?
Well I didn't bother to correctly indent the code examples, since I say that indentation is 4 spaces.

[sledgehammer999]

I am reopening this to discuss 2 more things that came to my mind:

• Initialization lists
  Each parameter should be on its own line.

class myClasss
{
public:
    myClass(int a, int b, int c, int d);

private:
    int priv_a;
    int priv_b;
    int priv_c;
    int priv_d;
};

myClass::myClass(int a, int b, int c, int d)
: priv_a(a),
  priv_b(b),
  priv_c(c),
  priv_d(d)
{
    //code
}

• Enums
  Each value should be on its own line.

enum days
{
    Monday,
    Tuesday,
    Wednesday,
    Thursday,
    Friday,
    Saturday,
    Sunday
};

Why?
I think it makes reading diffs when adding/removing values/parameters far easier to understand.

What do you think?

PS: If we go vertical what is best?

myClass::myClass(int a, int b, int c, int d)
: priv_a(a),
  priv_b(b),
  priv_c(c),
  priv_d(d)
{
    //code
}

//OR

myClass::myClass(int a, int b, int c, int d)
: priv_a(a)
, priv_b(b)
, priv_c(c)
, priv_d(d)
{
    //code
}

[glassez]

PS: If we go vertical what is best?

Second. In this case it is easier to insert a conditional Directive at the end of the list.

myClass::myClass(int a, int b, int c, int d)
    : priv_a(a)
    , priv_b(b)
#ifdef SOME_DEF
    , priv_c(c)
    , priv_d(d)
#endif
{
    //code
}

And, by the way, indented, too.

[sledgehammer999]

OK. So you agree with going vertical?
Because I don't know if you got my question, I am tagging @Gelmir @sorokin @pmzqla

[sledgehammer999]

@glassez
Should the enums be indented too?

[glassez]

Should the enums be indented too?

Yes. In your example enum is intended, so I decided not to talk about it.

So you agree with going vertical?

Yes. I agree with vertical enums, but for initialisers - optional.

[Gelmir]

I agree with vertical enums and initializers. As for comma placement - I guess this is a matter of preference.

[pmzqla]

I really don't have a strong preference on this that I could justify in any way, so whatever you choose, it's fine for me.

[Gelmir]

Here's Qt Creator config. Not much to look at.

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE QtCreatorCodeStyle>
<!-- Written by QtCreator 3.2.82, 2014-11-26T02:08:28. -->
<qtcreator>
 <data>
  <variable>CodeStyleData</variable>
  <valuemap type="QVariantMap">
   <value type="bool" key="AlignAssignments">true</value>
   <value type="bool" key="AutoSpacesForTabs">false</value>
   <value type="bool" key="BindStarToIdentifier">false</value>
   <value type="bool" key="BindStarToLeftSpecifier">true</value>
   <value type="bool" key="BindStarToRightSpecifier">false</value>
   <value type="bool" key="BindStarToTypeName">true</value>
   <value type="bool" key="ExtraPaddingForConditionsIfConfusingAlign">false</value>
   <value type="bool" key="IndentAccessSpecifiers">false</value>
   <value type="bool" key="IndentBlockBody">true</value>
   <value type="bool" key="IndentBlockBraces">false</value>
   <value type="bool" key="IndentBlocksRelativeToSwitchLabels">false</value>
   <value type="bool" key="IndentClassBraces">false</value>
   <value type="bool" key="IndentControlFlowRelativeToSwitchLabels">true</value>
   <value type="bool" key="IndentDeclarationsRelativeToAccessSpecifiers">true</value>
   <value type="bool" key="IndentEnumBraces">false</value>
   <value type="bool" key="IndentFunctionBody">true</value>
   <value type="bool" key="IndentFunctionBraces">false</value>
   <value type="bool" key="IndentNamespaceBody">true</value>
   <value type="bool" key="IndentNamespaceBraces">false</value>
   <value type="int" key="IndentSize">4</value>
   <value type="bool" key="IndentStatementsRelativeToSwitchLabels">true</value>
   <value type="bool" key="IndentSwitchLabels">false</value>
   <value type="int" key="PaddingMode">1</value>
   <value type="bool" key="SpacesForTabs">true</value>
   <value type="int" key="TabSize">4</value>
  </valuemap>
 </data>
 <data>
  <variable>DisplayName</variable>
  <value type="QString">qBittorrent</value>
 </data>
</qtcreator>

[glassez]

@sledgehammer999 CODING_GUIDELINES.md has errors - after for, while, if etc. must be space.

[pmzqla]

What about camelCase vs snake_case?
It seems that both are used in the code. IMO the code would look better using one of the two.

[sledgehammer999]

In the last files I have worked the past months camelCase seems much more prevalent so we should use that.
The only exception is a class's member variables which should be m_<variablename> to make them easily distinguishable that they are part of the class and not a local variable.
PS: The other exception is of course the webui function-name that are expected by ADD_ACTION() macro.