Trying to define our Code Style

Synfig code was and is written by many hands. To improve readability, maintanance and consistency, we should set our coding style.
In the old wiki, there is a short page about it, written in 2010 by @Genete, but it lacks a lot of info.
Currently, there are many of Code Styles used by many (modern) C++ projects, like Coding Guidelines | Haiku Project, Google C++ Style Guide and Code Style Guidelines | WebKit.

This topic is about building our own code style or following one of the existent ones to be used on new code contributions/commits and, in a future or hypothetical dream, on the entire Synfig code.

I’ll create here some polls about points the code styles mentioned above handles to we choose what fits better to our taste. The “results” will be compiled in http://synfig-docs-dev.readthedocs.io/ .

6 Likes

Recurrent debate, see argumentation here on the “dangers”

For me the most problematic is a kind of obsession in “optimizing” the source code.
Complex conditional expressions or constructors as parameters in functions…

Instead it could be decomposed in several lines, introducing temporary variables, to make it more readable and mistakes more obvious.

I used to be an industrial developer having to maintain and adapt neither documented nor commented copy-pasta source with thousands of lines per function that my colleagues didn’t even want to touch, scared to introduce side-effects.
A clearer code avoids this, indentation and vertical alignment to make look the code like a musical partition.
It also means avoiding auto-linting :stuck_out_tongue:

For example, what about to extract the calculations from this matrix:

So for me the most important is readability and simplicity.

1 Like

Yes, tab vs. space promotes wild debates. I won’t go through it. lol Mainly because almost every Synfig file uses tab. Only some lines here and there were mixed with spaces. Better leave this discussion for the future, if needed.

Other points I think we don’t need to discuss (at least for now) are file name extensions (.h, .hpp, .cc, .cpp, …) and C++ version, for example.

1 Like

About Header files

  • They must be idempotents, i.e. they must have include guards

  • They must be self-contained

  • At the same time they must be self-contained, they must #include only what is needed to be compiled by itself

  • About the include guard: the #define symbol must not start with double underscore, as it is reserved [1] [2] [3]

    • Synfig does not respect it. First poll:
  • Remove the preceding double underscore like in #define __SYNFIG_ROTATE_H as it is
  • Keep them
0 voters

Other existent problem is that they are not consistent for the synfig-studio/src/gui files.

  • ETL uses __ETL__XXXXX_H
  • synfig-core uses __SYNFIG_XXXXX_H
  • synfig-studio/src/synfigapp uses __SYNFIG_APP_XXXXX_H
  • synfig-studio/src/gui mixes __SYNFIG_STUDIO_GTKMM_XXXXX_H, __SYNFIG_GTKMM_XXXXX_H and __SYNFIG_STUDIO_XXXXX_H.

Second poll is: What should be the identifier style used for Synfig Studio GUI?

  • __SYNFIG_STUDIO_GTKMM_XXXXX_H
  • __SYNFIG_GTKMM_XXXXX_H
  • __SYNFIG_STUDIO_XXXXX_H
  • __SYNFIGSTUDIO_XXXXX_H
0 voters

Finally, #include order: alphabetically or categorized like Google Code Style proposes:

Include headers in the following order: Related header, C system headers, C++ standard library headers, other libraries’ headers, your project’s headers.

  • #include sorted alphabetically
  • #include order: Related header, C system headers, C++ standard library headers, other libraries’ headers, synfig project’s headers.
0 voters

@ice0 @BobSynfig @KonstantinDmitriev @Keyikedalube @FirasH

1 Like

I skimmed through the pages you linked above.

  • Google C++ Style Guide is comprehensive and a good should-read for C++ devs.
  • Guidelines | Haiku Project is visually appealing for C++ coders. Readability is prioritized. It is similar to GTK C Coding Style
  • Code Style Guidelines | WebKit is the one I am most familiar with. Easy because I’m using most of the default automatic indentation done by my IDE without having to worry too much about maintaining extra column spacings like GTK C or Haiku style.

Based on my experience contributing to Synfig GUI end I think most of the Webkit style can be implemented with a little mix of Haiku for column spacings since GTKMM tend to have lengthy identifier types

	Gtk::Grid        *grid_content;
	Gtk::Grid        *grid_canvas;
	Gtk::SpinButton  *width;
	Gtk::SpinButton  *height;
	Gtk::EventBox    *canvas_label;
	Gtk::CheckButton *canvas_only;
2 Likes

Still about header files, Synfig has templates for header and implementation files.

They present comment lines to visually help the file ‘sections’ and to guide the contents (like methods/variables/signals) order. Some class header files have more ‘sections’.

/* === H E A D E R S ======================================================= */

/* === M A C R O S ========================================================= */

/* === T Y P E D E F S ===================================================== */

/* === C L A S S E S & S T R U C T S ======================================= */
  • We should keep using those ‘section’ lines (and place them on the files that don’t have them yet/anymore)
  • We should remove them all (it doesn’t release us of following some contents order)

0 voters

Indentation

Moving to next topic: indentation. I won’t start a discussion about the TAB vs. spaces (at least for now) XD
I’ll focus on how to indent stuff:

  1. Namespaces and its contents:
    Do not indent. Google, Webkit and Haiku follow this rule.
    Good example:
namespace synfig {

class BLinePoint : public UniqueID
{
};

}

Bad example:

namespace synfig {

	class BLinePoint : public UniqueID
	{
	};

}
  • Namespace contents must use indentation
  • Namespace contents must NOT use indentation

0 voters

  1. Regarding switch statements, the case label should be indented according to Google and Haiku, but not according to WebKit and Qt.

WebKit and Qt style:

switch (condition) {
case fooCondition:
case barCondition:
    i++;
    break;
default:
    i--;
}

Google and Haiku style:

switch (condition) {
    case fooCondition:
    case barCondition:
        i++;
        break;
    default:
        i--;
}
  • Indent switch-case labels (Google & Haiku)
  • No indentation on switch-case labels (WebKit and Qt)

0 voters

Haiku and WebKit do not suggest to indent class access modifiers (public, protected and private). Google, however, explicitly says they should be indented by one single space.

  • Indent class access modifiers (public, protected and private)
  • Do not indent them

0 voters

1 Like

Sorry, but I’ll ping you again XD @ice0 @BobSynfig @KonstantinDmitriev @Keyikedalube @FirasH

2 Likes

Spacings

(This post is very inspired from WebKit docs), reusing some texts and examples from them.

Unary Operators

  • Do not place spaces around unary operators (WebKit, Google, others do not say explicitly):
Right:
i++;
if (!b) {}
Wrong:
i ++;
if (! b) {}

Assignment, Binary and Ternary Operators

y = m * x + b;
c = a | b;
return condition ? 1 : 0;
  • Usually: not mandatory for factors (product and division may be like a*b, and not necessarily a * b) (Google)
y = m*x + b;
f(a, b);
c = a | b;
return condition ? 1 : 0;
  • Never for assignment and binary operators (original Synfig code, but now it’s mixed with previously mentioned styles.)
y=m*x+b;
c=a|b;
return condition? 1 : 0;
For binary and ternary operators

0 voters

For assignment operators (=)

0 voters

Commas and semicolons

  • Do place space before comma and semicolon?
Right (WebKit):
for (int i = 0; i < 10; ++i)
    doSomething();

f(a, b);
Wrong (WebKit):
for (int i = 0 ; i < 10 ; ++i)
    doSomething();

f(a , b) ;
  • Usually not: Google (see “General” and “Loop and Conditionals” subsections)
// For loops always have a space after the semicolon.  They may have a space
// before the semicolon, but this is rare.
for ( ; i < 5 ; ++i) {
  ...
Spaces before comma and semicolon
  • Never (WebKit)
  • Usually not (Google)
  • Don’t care

0 voters

  • Do place space after comma and semicolon?
    • Mandatory for WebKit;
    • Haiku only says explicitly for comma;
    • Google incentives it [1] [2];
    • In this case, Qt style follows Haiku vagueness.

So I won’t make a poll here.

Inside Braces and Parenthesis

Is space acceptable between function/method name and its argument parenthesis?
  • Yes: fun (a, b); is valid
  • No: fun(a, b); is the only valid style (WebKit)

0 voters

Is space acceptable inside function/method name surrounding its argument list?
  • Yes: fun( a, b ); is valid (Google accepts it)
  • No: fun(a, b); is the only valid style (WebKit)

0 voters

Lambda functions/expressions

Lambda expression style:
  • [](int a, int b) {} : No space between context and arguments (WebKit)
  • [] (int a, int b) {} : Single space between context and arguments

0 voters

1 Like

As you can see (from the vote count), no one knows which is better. Perhaps it’s better to leave it as is. Just like one unique Synfig style feature?

2 Likes

To have general rules is not a bad thing but source code is written by humans for humans and need to keep a bit of humanity too.
It’s not just to set a linter and it has to keep a bit of logic, air and poetry.

For example, here is how I code (yeah, VueJS, but you can catch what I mean)

this.details.lines.forEach( item => {
  if (!item.editingBarcode) this.$set(item, 'editingBarcode', false       )
  if (!item.barcodeModel  ) this.$set(item, 'barcodeModel',   item.barcode)
  if (!item.editingQty    ) this.$set(item, 'editingQty',     false       )
  if (!item.qtyModel      ) this.$set(item, 'qtyModel',       item.qty    )
  if (!item._showDetails  ) this.$set(item, '_showDetails',   false       )
})

Try to set a rule for this :stuck_out_tongue:

3 Likes

Naming

cases

Current convention on Synfig’s code is:

  • classes/structs: CamelCase
  • unions: CamelCase (we don’t have any named union)
  • enumeration type: CamelCase
  • enumeration values: UPPERCASE → snake_case
  • variables/properties: snake_case
  • functions/methods: snake_case
  • constants: UPPERCASE → snake_case
  • macros: UPPERCASE
  • ‘templated’ types in ETL templates: snake_case (e.g. value_type)

My suggestion is to change the case of constants and enumeration values to snake_case.
Reasoning: avoid clashes with macros.
Example: deprecated Gtk::Stock has a value named DELETE, but it clashes with a macro defined in MS Windows headers. Solution was a hack:

The usage of lower case is supported by ISO-C++ foundation for enums and constants.

Case for enumeration values
  • Keep them UPPERCASE (example: SideType::TYPE_ROUNDED)
  • Change them to snake_case (example: SideType::type_rounded)

0 voters

Case for constants
  • Keep them UPPERCASE (example: SAMPLES)
  • Change them to snake_case (example: samples)

0 voters

Prefixes/suffixes

enum values:

Must we really add a prefix for enumeration values?
  • Yes (example: SideType::TYPE_ROUNDED)
  • No (example: SideType::ROUNDED)

0 voters

private/protected class variables:

To avoid shadowing and some compiler and logic issues besides worse readability in some cases, sometimes private variables/properties of a class have m_ as a prefix (e.g. m_my_var) or a _ as a suffix (e.g. my_var_) in Synfig code. Sometimes it simply allows variable shadowing (e.g. my_var).

Another opensource project written in C++, Godot Engine prefers to solve it by prepending p_ to method parameter names:

How should be name private and protected class variables/properties?
  • Prefixed with m_ (from “member”) (example: m_color) (barely used in Synfig, mostly in a random way)
  • Suffixed with _ (example: needs_sync_) (often used)
  • Simply don’t use any prefix or suffix and allow variable shadowing (often used)
  • Force all class method parameter names with a p_ prefix (from “parameter”) (example: set_color(const Color& p_color))

0 voters

I’m not sure that we will get new contributors if they have to assimilate and apply so many rules in addition of learning the global architecture of the application which is, let’s admit it, still poorly documented…

We need diagrams to understand the code, not to scare people :stuck_out_tongue:


1 Like

Sadly we have so few for years even without any code style actually set.
Anyway this isn’t the aim on setting our code style.

About the number of rules: they are not so many yet. Many of them are well-established in Synfig code.
Anyway, we can use technology on our side: source code editors have template styles and code refactoring; there are code ‘formatters’ like ClangFormat, and we can use them via git pre-commit hooks, GitHub actions, etc. so we don’t spend too much time on it and still succeed on getting the code easy to read and avoiding some traps.

1 Like

Yes, it is. Not too difficult to admit it :frowning:

This is precisely what I am warning about.
Source code should be self-sufficient and well-structure, not post-processed by automated tools in order to keep the mind and structure of the original creator.
The only tools we should rely on are the suggestions done by the IDE at write time.

About pre-exiting “style”, well…
When I contribute, I tend to follow implicitely the conventions existing inside the original code (unless my work is precisely to restructure or refactor old code like I had in some jobs)

We should focus more on tips to newcomers like:

  • Don’t make too much long lines, break in several (like in Lottie plugin, where a directive to skip line
    length check is set)
  • Should we decompose long instructions into several lines to improve readability and debugging possibilities
  • Should we use “Yoda conditions” to avoid to read unneeded parts of the code
  • Mark regions as such (with code folding if possible
  • Extract code from conditionnal structures and place them in separated functions (readability, debugging)
  • Comments, comments, comments…

Too strict code formatting rules bring rigidity and “traditionnal” copy-paste-and-modify-from-StackOverflow-because-it-worked-for-someone-else mistakes.

And if really some contributions are really too badly written, we can still request politely to clean them before PR to be applied, which is great for the developer to learn and improve his skills in writing better code :wink:

P.S.: This reflects my own idea of coding, I am not deciding for the project.

Hmm… Not everyone writes some way expressing a poetry like you said before, but sometimes in a hurry or just a mess, mixing styles in his/her own code.

When you say “original creator” you mean “project creator”, “file creator” or “that code slice creator”? darco had a style, other contributors mixed when contributed into his files, blackwarthog (and me and others) wrote in other styles when created new files, and others than used another style in their turn.

Some projects use style formatters and they don’t screw the code up and don’t make it a barrier to contributors. Example: Godot Engine.

Some do, some don’t set up the IDE properly. And we would need to set up the IDE style differently depending on the file (or class method) style…

Well, like I said in previous post, that is not the reason I’m proposing a code style for this project.
Some reasons are: consistency, readability, visual comfort, avoiding code mistakes, setting up and using a single style in the IDE/editor, etc.

For that, a minor effort from the contributor (or none) would be needed.

Some many comments usually means the code is bad. :wink: Like you listed before, split into functions/lines and most comments are unneeded.

I’m not deciding anything here :slight_smile: I’m collecting opinions (almost finishing it) and then I’ll propose a code style. If approved, I’ll write it down in the Dev Docs (focused on examples instead of text), create a CLangFormat style file (and other beautifiers you guys suggest), write a pre-commit git hook and, maybe, a Github action.

1 Like

Hey guys, I’m a new contributor here so I thought I could maybe add some insight on what @BobSynfig mentioned regarding new contributors, and how all this might affect the process of having new contributors integrate into the synfig contribution process.

I personally believe that as long as most of the style stuff is post processed by an automated tool, then this shouldn’t pose any issues that would make it more difficult to get started contributing. The only case that would make it more difficult is if there were many code style practises set in place and have to be followed rigorously and manually, then it might make the process require more work from the contributors part and maybe some contributors don’t have much time or the patience perhaps. However from what it seems this is not the case, so again I don’t see this affecting new contributors in any bad way. Also in many other projects there are coding styles set for everyone to follow so the concept itself shouldn’t be too foreign to anyone new to open-source Dev or Dev in general.

These are my two cents guys, of course not all new contributors may think like this but I believe a good portion might, which is why I shared it. I hope it added something of use to this discussion :smile:.

2 Likes

Pointers and references

char* name or char *name?
std::string& name or std::string &name?

The * and & characters must (or should) be next to the type (char* ) rather than to the variable name ( *name) according to Chromium, C++ Core Guidelines and WebKit. Haiku does not explicitly says so, but all of its examples use this style too.
However, other C++ coding style require the inverse: Google coding style (only?). Qt does not explicitly state anything about it, but its examples use this second style.

So here is the poll, split in two just in case:

Pointer mark position
  • int* : Pointer types MUST NOT have a space between type and *
  • int *: Pointer types MUST have a space between type and *

0 voters

Reference mark position
  • int& : Reference types MUST NOT have a space between type and *&
  • int &: Reference types MUST have a space between type and &

0 voters

The line for Return type of functions and class methods

In Synfig code, the implementation files (*.cpp) usually (and in their majority) have the return type of functions or class methods in the line before its function/method name and parameter list.
Example 1:

Layer::Vocab
Layer_Bitmap::get_param_vocab()const
{
	Layer::Vocab ret(Layer_Composite::get_param_vocab());


	ret.push_back(ParamDesc("tl")
		.set_local_name(_("Top-Left"))
		.set_description(_("Upper left-hand Corner of image"))
		.set_is_distance()
	);

Example 2:

inline
const Color&
synfig::Layer_Bitmap::filter(Color& x)const
{
	Real gamma_adjust(param_gamma_adjust.get(Real()));
	if(gamma_adjust!=1.0)
	{
		x.set_r(powf((float)x.get_r(),gamma_adjust));
		x.set_g(powf((float)x.get_g(),gamma_adjust));
		x.set_b(powf((float)x.get_b(),gamma_adjust));
		x.set_a(powf((float)x.get_a(),gamma_adjust));
	}
	return x;
}

I found some reasoning for it in Stack Exchange.
Some Synfig files completely ignore this style.
What style should we follow?

Return type for function implementations
  • The line before the function or method name (like in both examples above)
  • The same line the function or method name (like this and this)

0 voters

Back to indentation: preprocessor directives

Should we indent them? Like this:

#ifdef USING_PCH
# include "pch.h"
#else
#ifdef HAVE_CONFIG_H
# include <config.h>
#endif
#endif

Google coding style allows it (it isn’t mandatory). I could not see any other commenting about it.
Please note it’s not about indenting the hash character (#), but what follows it.

Preprocessor directive indentation
  • Indent after hash (#), like example above
  • Don’t indent

0 voters

After this post, there are only 3 more topics I’d like to ask about (finally!): braces, documentation and statement styles.

Return type on previous line: Clearer and shorter lines
Indentation in preprocessor: I love proper indentation :stuck_out_tongue:

Pointers and references: I always prefered the second form because it looks more “safe”

See here the comment of Brian Overland

There is no difference EXCEPT for the following situation. Suppose you are defining multiple pointer variables. Compare and contrast these two lines:

1. char *p, *p2, *p3; // This does what you think it does.
2. char* p, p2, p3; // This does not do what you think it does.

These two lines do very different things. The second line here declares p as a pointer variable, but p2 and p3 are not declared pointers, just ordinary variables of type char. This is not technically wrong (other than the fact simple variables of type char are not especially reliable or useful); it is just highly misleading.

The second style above — to put the * closer to the type name — appears often in code but has the possibility of creating misleading declarations when declaring multiple pointer vars, as shown above.

By the way, there is a way to achieve the effect that is probably desired by that second line above. First, use typedef to create a pointer type.

1. typedef char *CPTR;

Now you can declare multiple pointers more efficiently:

1. CPTR p, p2, p3;

1 Like

In that case, I vote for only one variable per line, and preferable with its initialization. Otherwise it is unsafe too. :slight_smile: Also variable declarations next to their first use helps to avoid the mentioned problem too.