Trying to define our Code Style

About consistency and readability
Let’s take for example synfig/layer.cpp

We have a lot of different styles of for loops with iterators.
The line containing the for are quite long
Some with the instanciation of the counter outside (which i personaly prefer) some directly inside.

L236
for(DynamicParamList::const_iterator i = dynamic_param_list().begin(); i != dynamic_param_list().end(); ++i)

while L335-336

DynamicParamList::const_iterator iter;
		for (iter = dynamic_param_list().begin(); iter != dynamic_param_list().end(); iter++)

Look already more readable.
I would even split the for over 3 lines here.
Long lines are tiring (I’m from the old school, Turbo Pascal text mode 80x25 and compiler limited to 127 chars per line :P)

Line 672, declaration with 168 chars on one line, ouch:
Layer::render_transformed(const Layer *layer, Context context,Surface *surface,int quality, const RendDesc &renddesc, ProgressCallback *cb, const char *file, int line)

As written here, “Readability is a valid reason to learn to use whitespace” :stuck_out_tongue:

After no need to fall in the reverse attitude:
from very compact (L 1059):
monitor_connection = file_monitor->signal_changed().connect(sigc::mem_fun(*this, &Layer::on_file_changed));
to fully exploded (L 217-225):

			parent_death_connect_=x->signal_deleted().connect(
				sigc::bind(
					sigc::mem_fun(
						*this,
						&Layer::set_canvas
					),
					etl::loose_handle<synfig::Canvas>(0)
				)
			);

where sigc::mem_fun( *this, &Layer::set_canvas ),
could have been sufficiently readable.

IMHO it is a matter of context.

Plus this is open-source, multi-developers and multi-reviewers, let’s make it simple to understand for maintainance and new contributors :slight_smile:

1 Like

Initially, I’ve been using the 2nd pointer style:

int *pointer;

Later down the road coding C++ I realized the language emphasizes types a lot:

signal_event.connect(sigc::bind<T1&, T2*>(sigc::connect(...

Adding spaces to types T1 and T2 reference and pointer symbol respectively would have implied identifier names should have followed.
C++ Core Guidelines link posted above explains why. Found a similar answer in this StackOverflow answer.

2 Likes

Documentation comments

Originally, Synfig code uses annotations/comments to document its API.
Doxygen was/is the tool to generate the documentation files from it, although they were never available on Synfig website: who wants to see the API docs must generate by him/herself.

I don’t know any other tool to do such stuff. Most of C++ projects use it. By their turn, others create the docs completely apart from code (such as Godot Engine). However, they have a dedicated team to do it: we barely have developers :frowning: . So I vote to keep using Doxygen and its annotation strategy to document code.

How to document Synfig API and code
  • Keep using Doxygen annotations
  • Other way (What way? Please write.)

0 voters

If we keep using Doxygen, could we try to uniform the style? Doxygen is very tolerant with doc style.
It has its own style, but accepts Java doc and Qt style also, for example.

So let’s vote what is the preferred way of the comment block to doc :

JavaDoc style:

/**
 * ... text ...
 */

Qt style:

/*!
 * ... text ...
 */

Third style (C++ comment style + / char)

///
/// ... text ...
///

Fourth style (C++ comment style + ! char)

//!
//! ... text ...
//!
  • JavaDoc style (comment block //* * *//)
  • Qt style (comment block //! * */)
  • Third style (comment lines ///)
  • Fourth style (comment lines //!)

0 voters

If JavaDoc or Qt style are the preferred way, these comment blocks should use * char on each intermediate line as shown in the examples above?

  • Start every intermediate line of the comment block with * char
  • Don’t use that * “prefix”

0 voters

For coherence, Putting documentation after members should follow the same style voted above.

Finally, when commenting class methods, for example, we can use special commands like param and return to describe each parameter and return value.
Example #1 (synfig-studio/src/gui/duckmatic.h):

	//! Return list of box contained ducks (handles). The box is defined by a vector's pair
	/*!
	** \param tl The top left canvas coordinate has const synfig::Vector
	** \param br The bottom right canvas coordinate has const synfig::Vector
	** \return ducks (handles) has const DuckList
	** \sa toggle_select_ducks_in_box, select_ducks_in_box, find_duck
	*/
	DuckList get_ducks_in_box(const synfig::Vector& tl,const synfig::Vector& br)const;

(\sa means “see also”)

Example #2 (synfig-core/src/synfig/matrix.h)

	//!set_scale member function. Sets a scale matrix
	//! @param sx Scale by X axis
	//! @param sy Scale by Y axis
	//! @return A matrix reference filled with the sx, sy values
	Matrix2& set_scale(const value_type &sx, const value_type &sy);
What command style should we use?
  • \param
  • @param

0 voters

Next post will be the last one I intend to open here, handling braces and statement styles.

In some cases, there are big sections of code that don’t need to be seen all the time.
I use sometimes code folding in Netbeans (not standard sadely).
Useful when there are several thousand lines per file :stuck_out_tongue:

// <editor-fold defaultstate="collapsed" desc="user-description">
  ...any code...
// </editor-fold>
1 Like

I just checked and IntelliJ Idea uses this tag too:

My final (and looooong) polling post, I hope :slight_smile:

Braces

The focus is curly braces. They are used a lot in C++ as we can see below:

for (const auto& item : list) {
}

while (something) {
}

do {
} while (something);

if (something) {
}

switch (value) {
case 1:
    ...
}

class MyClass {
};

enum MyEnum {
};

void my_function() {
}

auto my_lambda = [] () {
};

curly braces for empty control clauses

I think it is unnecessary to ask about it, but here we go: when a control clause has an empty body, can we use semicolon ; or must we require to use empty braces { }?
Examples:

// Case 1:
while (condition)
    ;
// Case 2:
while (condition);
// Case 3:
while (condition)
    { }
// Case 4:
while (condition) { }

Both first 2 cases are dangerous, but case 2 is particularly more dangerous because it “hides” (more) the end of the control:

while (condition);
{
    // this is not a loop and it is always executed no matter the value of "condition" in "while" above
    do_it();
}
Empty control clauses
  • We may use semicolon ; (we allow cases 1 and 2)
  • We must use empty curly braces { } (only cases 3 and 4 are acceptable)
0 voters

to use or not to use

Must the use of curly braces be mandatory for statements with a single line control or are they optional in this case?

// If braces are optional, these are okay:
// If they are forbidden, only this style is allowed for single-line bodies:
if (condition)
    do_it();

while (condition)
    do_it();

for (;;)
    do_it();

// If braces are optional, this is okay too:
// If mandatory, previous statement must be this way:
if (condition) {
    do_it();
}

while (condition) {
    do_it();
}

for (;;) {
    do_it();
}

For single line control clauses
  • Braces are optional
  • Braces are mandatory
  • Braces are forbidden
0 voters

No matter what wins above, we refer only to single-line bodies: comments and multiple-lines commands must always use curly braces:

// Here is mandatory
if (condition) {
    // Some comment
    do_it();
}

if (condition) {
    my_function(reallyLongParam1, reallyLongParam2, ...
        reallyLongParam5);
}

This is supported by Qt, WebKit, Google and Haiku.

brace symmetry

For the if statement, we may have the else clause. If curly braces are not mandatory, we may have asymmetric use of {} if if clause has only a single line but else clause has not (or vice-versa).
Example:

if (condition)
    do_it();
else {
    do_other_thing();
    and_more();
}

Should we enforce brace symmetry in those cases? Qt (in “exception 2” subsection) and Google Style (in the last examples) emphasize it. WebKit does not: one “right” example makes an asymmetric usage like this:

if (condition)
    doSomething();
else {
    ...
}
Brace symmetry
  • Mandatory
  • Not mandatory
0 voters

Brace lines

What line the starting (and ending) curly brace must be placed on?

WebKit

WebKit requires that the open brace must be placed on the line preceding the code block, except for function definitions:

// WebKit style

class MyClass {
    ...
};

namespace WebCore {
    ...
}

for (int i = 0; i < 10; ++i) {
    ...
}

if (condition) {
    ...
} else {
    ...
}

// functions behave differently:
int main()
{
    ...
}

Haiku

Haiku is not explicit, but its examples use a similar style WebKit uses: the difference is that, for classes, open brace is placed on the next line:

// Haiku style

namespace WebCore {
    ...
}

for (int i = 0; i < 10; ++i) {
    ...
}

if (condition) {
    ...
} else {
    ...
}

// functions and classes behave differently:
int main()
{
    ...
}

class MyClass
{
    ...
};

Google

Google wrote some points for function definitions:

  • The open curly brace is always on the end of the last line of the function declaration, not the start of the next line.
  • The close curly brace is either on the last line by itself or on the same line as the open curly brace.

For the rest, the document doesn’t say anything, but we can see they follow WebKit style in their examples.

Qt

Qt docs requires that the left brace on start of the next line only for class declarations and function definitions (lambda not included).

So it is styled as Haiku is.

Synfig

Synfig mixes it. As far as I can see, it originally used always open and close curly braces in a separate line:

Let’s vote!

I’ll split here for voting:

Loops
// option 1:
for (const auto& item : list) {
}

while (something) {
}

do {
} while (something);

// option 2:
for (const auto& item : list)
{
}

while (something)
{
}

do
{
} while (something);
Brace position for loops
  • for, while and do-while have the open/left curly brace { in the same line of the control statement (Qt, WebKit, Google, Haiku)
  • for, while and do-while have the open/left curly brace { in the next line (starting the line) (Synfig, sometimes)
0 voters
Enum
// option 1:
enum MyEnum {
    ITEM_1,
    ITEM_2,
};

// option 2:
enum MyEnum
{
    ITEM_1,
    ITEM_2,
};
Brace position for enum
  • enum must use the open/left curly brace { in the same line of the control statement (Qt, WebKit, Google, Haiku)
  • enum must use the open/left curly brace { in the next line (starting the line)
0 voters
Namespace
// option 1:
namespace {
    ...
}

// option 2:
namespace
{
    ...
}
Brace position for namespace
  • namespace must use the open/left curly brace { in the same line of the control statement (Qt, WebKit, Google, Haiku)
  • namespace must use the open/left curly brace { in the next line (starting the line)
0 voters
Classes (and structs)
// option 1:
class MyClass {
};

// option 2:
class MyClass
{
};
Brace position for class definitions
  • class must use the open/left curly brace { in the same line of the control statement (WebKit, Google)
  • class must use the open/left curly brace { in the next line (starting the line) (Qt, Haiku)
0 voters
regular functions
// option 1:
void my_function() {
}

// option 2:
void my_function()
{
}
Brace position for function definitions
  • functions must use the open/left curly brace { in the same line of the control statement (Google)
  • functions must use the open/left curly brace { in the next line (starting the line) (WebKit, Haiku, Qt)
0 voters
lambda functions
//option 1:
auto my_lambda = [] () {
};

option 2:
auto my_lambda = [] ()
{
};
Brace position for lambda function definitions
  • lambda functions must use the open/left curly brace { in the same line of the control statement (Qt, Google)
  • lambda functions must use the open/left curly brace { in the next line (starting the line) (none explicitly)
0 voters

(Haiku does not comment anything about lambda functions anywhere)
(WebKit does not say anything explicit, but the few (and single lined) lambda examples are inline)

if statement
Brace position for if statements
  • if statement must use the open/left curly brace { in the same line of the control statement (Qt, Google, WebKit, Haiku)
  • if statement must use the open/left curly brace { in the next line (starting the line)
0 voters
// option 1:
if (condition) {
    ...
} else { // same line
    ...
}

// option 2:
if (condition) {
    ...
}
else { // next line
    ...
}
Brace position for else statements
  • else statement must be in the same line of close brace: } else
  • else statement must be in the next line
0 voters
switch statements

None of mentioned code style say anything explicitly about switch.
We can discuss 2 parts:

  1. the mandatory braces for switch itself and
  2. the ‘optional’ braces for cases (mandatory if there are variable declaration in a case).
  • Switch itself
// option 1:
switch (value) {
case 1:
    ...
}

// option 2:
switch (value)
{
case 1:
    ...
}

Brace position for switch statements
  • switch statement must use the open/left curly brace { in the same line of the control statement (Qt, Google, WebKit, Haiku)
  • switch statement must use the open/left curly brace { in the next line (starting the line)
0 voters
  • case part
// option 1:
switch (value) {
case 1: { // on same line
    ...
}
}

// option 2:
switch (value) {
case 1: // { on next line
{
    ...
}
}

Brace position for case statements
  • case statement must use the open/left curly brace { in the same line of the control statement (Qt, Google, WebKit, Haiku)
  • case statement must use the open/left curly brace { in the next line (starting the line)
0 voters

Can control body be in the same line as control statement?

Is this acceptable? No matter whether using braces or not.
For if, for, while… Sometimes code is written like below in Synfig:

while (reading) { i++; } // Can we accept those single line statements?
if (!ready) return;
for (;;) { do_something(); ++i; }
Can control body be in the same line as control statement? Somehow related to empty body voted uuuup there.
  • No, we must not allow it
  • Yes, but only if it is just a single instruction
  • Yes, we allow it
0 voters

one variable declaration per line

static inline

As discussed in a previous post, the return type of a method implementation is written in the previous line of method name.
However, I forgot that sometimes there are some method specifiers like static and inline. Should they be listed in the line before the return type (example below) or in the same line as the return type?

static and inline specifiers for class method implementations:
  • In the same line of the return type (most cases in Synfig)
  • In the previous line of the return type (example above)
0 voters

Constructor initializer list :

On what line should we place the : for constructor initializer list?

Haiku

// The ':' always comes on its own line, initializers following
Foo::Foo(int32 param)
	:
	Bar(int32* param),
	fMember(param),
	fPointerMember(NULL)
{
	...
}

Google

// When everything fits on one line:
MyClass::MyClass(int var) : some_var_(var) {
  DoSomething();
}

// If the signature and initializer list are not all on one line,
// you must wrap before the colon and indent 4 spaces:
MyClass::MyClass(int var)
    : some_var_(var), some_other_var_(var + 1) {
  DoSomething();
}

// When the list spans multiple lines, put each member on its own line
// and align them:
MyClass::MyClass(int var)
    : some_var_(var),             // 4 space indent
      some_other_var_(var + 1) {  // lined up
  DoSomething();
}

// As with any other code block, the close curly can be on the same
// line as the open curly, if it fits.
MyClass::MyClass(int var)
    : some_var_(var) {}

WebKit

MyClass::MyClass(Document* document)
    : MySuperClass()
    , m_myMember(0)
    , m_document(document)
{
}

MyOtherClass::MyOtherClass()
    : MySuperClass()
{
}

C++ Core Guidelines

File_handle::File_handle(const string& name, const string& mode)
    : f{fopen(name.c_str(), mode.c_str())}
{
    if (!f)
        throw runtime_error{"File_handle: could not open " + name + " as " + mode};
}
Where should be the colon : for constructor initializer list?
  • On its own line (Haiku - see its example above)
  • At ‘start’ of initilizations line (the rest)
  • At the end of the constructor signature line (before the initialization line)
0 voters

AND THAT’S IT. WOW

@FirasH @ice0 @KonstantinDmitriev I finally list all polls I initially intended to create.
I’m waiting for more (yours) opinions to write the proposal of code style with examples and a clangformat file.

After a final approval, I’ll try to create a git pre-commit hook (and maybe QtCreator and NetBeans stylers)

4 Likes

Yep necroposting.

I think it’s worth to revive this. The variable name clash due to the line ‘using namespace std;’ is one of the things that is causing builds under Windows to fail.

Wouldn’t a dev doc page about coding style guidelines be appropriate/helpful for readers too? Inside the Code structure section.

Screenshot_2023-05-13_15-18-35

Or should it be in Contributing guidelines ?

I suggest write them via examples/templates. Show templates for if statement, for statement, class/struct declarations, enum, etc. And then, after each template, write some directions if needed/desired.

1 Like

I think contributing guidelines may make more sense as well. As new contributors would (hopefully) look there first.

1 Like

Guys I’ve opened the PR. It’s still in draft so criticisms for objection/modification are open.

3 Likes

As a new contributor, I don’t think these guidelines for coding styles are intimidating, but rather give proper direction to what my code should look like. It’s good to define standards because it reduces all the guessing and assumption from work and will lead to high quality work.
It will be a good thing for me, now that I know what is Code Style for Synfig, I will follow these guidelines throughout any contributions I may make thus improving the quality of work I do, and helps me follow a good system for writing code.