What about modernizing Synfig code

Synfig is an old project, with a lot of stuff from before C++ library standardization, even pre-C++11.
Many coding good practices were developed/adopted since then, but sadly this project didn’t follow those changes, for different reasons (fear of breaking what currently works, too few developers, large source code, support of old systems, etc.).

Many helpful people contributed to Synfig, but we don’t follow some code “rules”/“consistency” in our contributions along the time.
Some cases are simple and “irrelevant”, like coding style: what is the project “autodoc” style (via Doxygen): ///, //!, /**, etc?; or if we should place a space between if keyword and its following parenthesis; or if class private member names should be pre/appended with _.
Other situations are more problematic, made “by hand” (like for loops instead of a now standardized std::find()), being the code error prone or difficult to understand and, consequently, to maintain.

Not modernizing our code base can be a contributor-barrier too. It seems C/C++ are “not” the “dominant” programming language among new aspiring contributors. Many of them know better Python and other scripting languages, with many features that ease development, and without usage of modern C++ standard feels like a pain.

We also don’t have the habit of writing unit tests for old and new code. That would help to avoid/prevent regressions and to detect bugs we didn’t see by reviewing code or even in the already existent code (like this, that made me spent one entire day to realize what I was doing wrong on my code, but it was not my code fault).

In the end, C++11 is (still) our current version target. What about we do aim to use the potential of C++11?
I mean we should start to follow the C++ Core Guidelines.
It is maintained by Standard C++ Foundation and it states at least one reason for every recommendation they made in its Core Guidelines. They usually lists bad/good examples too.

My proposal is to create a new project in our repository and report issues related to the Guidelines and link them to this new project. Thus we will be able to triage our progress, prioritize our efforts and make this large source code more consistent and maintainable.

I know we are so few contributors to do it, but that’s exactly one of the reasons that makes me think about it. It could be an easy way to new people start to contributing to source code and know/understand/experience more slices of the software code. One example is the “rule” Enum.3: Prefer class enums over “plain” enums. The programmer would have to search for "\benum\b" in the source code (via git grep or IDE, perhaps) and make a (simple) analyzis if the rule applies or not to that case, checking maybe for the first time that code area (“hm… so that’s a code for exportation”). By the way, the \b I used in the search word represents word boundary for regular expressions.

So what do you guys think about it?


PS: I support we should use C++17: it is now the standard required by gtkmm4, libxml++ (4.0 and 5.0), libsigc++ (3.0). But maybe we can shift later than we adopt some C++11 good points and current good practices. It would even help us simplify (= remove?) our synfig::ValueBase, as c++17 introduces std::variant.

1 Like

Agree but this requires at least two or three dedicated developers working full time maybe during weekends to gain some momentum.
Right now I see only one developer (you) patching/improving/enhancing Synfig code. @KonstantinDmitriev and @ice0 are principal developers and they have more tasks on their plates so I excluded them.
C++11 is not too old just yet. Actually, C++11 is a good starting point to base Synfig code modernization on because of the project’s tendency to support old stable-stable Linux distros. This guarantees no build breakage for old systems.

C++ will always remain at the heart of Syfing’s backend because we need that low-level performance boost (render engines) and OOP features (scalable programmers productivity). However, GUI code written in GTKMM can be rethought to be written in Python in the future. IIRC @ice0 was onto this.
I haven’t any experience in PyGObject :frowning:

This includes me but here’s my two cents: Synfig coding style is not uniform so I just stick to whatever style the original author wrote in. Because I don’t want one source file to have this layout:

// original author wrote this way
...
if(condition)
{
    ...
} else
{
    ...
}

// against this which is actually my preferred choice
if (condition) {
    ...
} else {
    ...
}

I think Synfig dev docs is a good starting place to let contributors know about this stuff. Noticed there are tons and tons of standard for loops that can easily be replaced with range-based ones.

Again dev docs is a good starting place. In my college years we were never taught such stuff and I learned about it from real-world projects later on. I still don’t do them because I’m lazy :stuck_out_tongue:

Fully agreed :+1:

2 Likes

But now and then some others appear, like you and @FirasH. And these days we have some potential regular contributors in the forums and in Github. Some of the guideline items do not require an comprehensive understanding of Synfig code or C++. Besides, if we don’t start, it won’t happen.

Sadly, I agree. The sadness is due to the std::variant.

Possible, but I think the same crashes will happen. But maybe this would be the way to have a bug report with backtrace log lol

I confess I’m guilty here: sometimes I follow the file style, sometimes I use ‘mine’…
But I think we should make a task force for set one code style and force it. clang-format could help us here. Maybe even as a git-hook and as one of the github checkers.
Two code styles I propose to analyze: Code Style Guidelines | WebKit Google C++ Style Guide

Me neither, but I’m an electrical/electronic engineer XD
And it’s tedious to think and write.
However, it has a significant benefits.

2 Likes

a bit unrelated but so am I XD, glad I’m not the only one :smile:. Thought all here would be CS/CE. I’m actually studying electrical/electronics with a specialization in IC design (not yet determined if analog/digital but most likely it’ll be digital).

1 Like

I think this would be very doable also, actually while looking through the source code I noted some, especially style inconsistencies/extra spaces etc… and was thinking of jotting down their places to go back later on and possibly fix them. so I believe if everyone just added things they encounter in a project like the one you mentioned, it would be a move in the right direction and wouldnt take too much effort.

Sadly, I agree. The sadness is due to the std::variant.

variant.cpp

#include <variant>
cpp g++ -std=c++17 -E ./variant.cpp > ./variant.out
➜  cpp stat -c%s ./variant.out                        
280520

280K of code makes me sad too :slight_smile:

Anyway we should switch to c+±17 sometimes. We can switch tomorrow if it’s really needed, but I don’t see the need for it yet.

My current plan is:

  1. Fix critical errors (crashes, incorrect behavior)
  2. Simplifying and documenting the code (this will help new contributors a lot)
  3. Fix memory leaks. Unfortunately there is a known issue in sigc++ which don’t allow to use sanitizer to detect memory leak. This issue is fixed in sigc-3.0, so this leads us to 4.
  4. Modernize GTK. This should allow us to implement the following features: switchable themes, correct main menu for MacOS and the ability to use GTK4.
  5. Performance issues.
  6. Unit-tests.

Also I don’t think the style is a big issue. I think all styles can be fixed with clang-format automatically in the CI stage (except variable names, maybe?).

3 Likes
$ cat teste-valuebase.cpp 
#include <synfig/value.h>
$ g++ -E ./teste-valuebase.cpp -I synfig-core/src/ -I ETL/ -I _production/build/include/ETL/ > ./teste-valuebase.out
$ stat -c%s ./teste-valuebase.out 
1705219

Whoops :slight_smile:

I do like your plan :slight_smile: , and I don’t see any conflict.
To prevent and detect errors and memory leaks, and to simplify code are what unit-tests and that Core Guidelines aim at.

No, it isn’t, but it makes the source code a bit messy and confusing to read IMHO.
What about:

  1. We first set our code style? New contributions/commits should obey it. I propose to base ours on Webkit code style I linked above. We could depict it here in forums and vote/debate item by item.

  2. Related to code simplification, we could start by ETL:

    • replace smart_ptr
    • remove useless etl_config.h
    • don’t separate its files in without/with .h: I can’t see why it’s this way now
    • move away to synfig-core non-template stuff, like angle, clock, misc and stringf
    • split stringf: filepath related stuff should not be there
    • compare misc’s binary_find vs. std::binary_search and do something about it
    • cry and whine about handle and reference_count
    • move everything else to synfig-core

Regarding errors, I support unit-tests and/or automatic tests (the latter would be easier to create with a scripting language :frowning: ). And follow those c++ guidelines lol

1 Like

<troll>
Let’s rewrite for Qt
</troll>

1 Like

I know you were trolling, but if that was the solution I’d support it (although I have my constraints about Qt, as I always did about Java).


The Amazing World Of Gumball | The Game: Nicole’s Head Of Doubtness
Fandom Wiki

Current status:

  • replace smart_ptr (#2686)
  • remove useless etl_config.h (#2733)
  • don’t separate its files in without/with .h: I can’t see why it’s this way now
  • move away to synfig-core non-template stuff, like angle (#2715), clock (#2711), misc and stringf (WIP: #2735)
  • split stringf: filepath related stuff should not be there (WIP: #2710)
  • compare misc’s binary_find() vs. std::binary_search() (or std::lower_bound()) and do something about it
  • cry and whine about handle and reference_count
  • move everything else to synfig-core

Regarding Trying to define our Code Style, I already wrote a doc summarizing and exemplifying the voted style. I’m coding trying to follow it and checking some stuff. (Example: I missed to ask the style for single-line doc comment ).

1 Like