Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Canvas class: hide std::list methods and do some cleanup #2364

Merged
merged 10 commits into from Oct 15, 2021

Conversation

rodolforg
Copy link
Contributor

@rodolforg rodolforg commented Oct 14, 2021

Although Canvas itself is (argh) a std::list of layers, it must always have a forced last list element: a null layer.
As a consequence, it must override some std::list methods to assure a programmer doesn't access or remove this special layer: like size() and end().

However, std containers are not supposed to be derived as they don't have virtual methods. A misplaced cast (like passing a Canvas as argument to a function that accepts std::list as parameter) may broken the list.
As original code was written way before C++11, some methods were added to std::list (and more may will be added). A distracted programmer could use those new methods and break the special behavior of this derived std::list. Example: push_back() with different signatures of the original - and they do exist now on C++11: current implementation only overrides one.

Therefore, it is best to just hide this ancestral class by making this inheritance private and make it available to programmer only via wrapped or customized methods.

@rodolforg
Copy link
Contributor Author

OK. Autotools in my machine build.
Errors were in CMake. Any idea?

@ice0
Copy link
Collaborator

ice0 commented Oct 14, 2021

OK. Autotools in my machine build. Errors were in CMake. Any idea?

Checking now.

@ice0
Copy link
Collaborator

ice0 commented Oct 14, 2021

OK. Autotools in my machine build. Errors were in CMake. Any idea?

That's just a luck )
Same error with autotools build:

make[2]: *** [Makefile:1317: action_export_icon.png] Segmentation fault (core dumped)
make[2]: *** Deleting file 'action_export_icon.png'
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [Makefile:1317: action_unexport_icon.png] Segmentation fault (core dumped)
make[2]: *** Deleting file 'action_unexport_icon.png'

it faults on different files, so you need to restart it several times to reproduce.
But for me it just hangs.

You can test it using this command:

./synfig ~/projects/synfig/synfig-studio/images/action_interpolate_interpolation_icon.sif -o ./test.png --time 0f

I noticed that it loops in remove_group_pair method, so I added debug output to canvas.cpp:

Canvas::remove_group_pair(String group, etl::handle<Layer> layer)
{
	static int counter = 0;
	std::cout << "Removed from: " << group << ++counter << std::endl;

and this is result:

Removed from: Shading13832
Removed from: Shading13833
Removed from: Shading13834
Removed from: Shading13835
Removed from: Shading13836
Removed from: Shading13837
Removed from: Shading13838

It hangs because layer inside group_db_ is NULL, so it never exits (I guess because of the recursive call).

@rodolforg
Copy link
Contributor Author

That's just a luck )
Same error with autotools build:

Quite interesting. Here a I could build Synfig Studio, open it and even load some files... :)

@rodolforg rodolforg force-pushed the pseudo-random-cleanup branch 2 times, most recently from 36609d7 to f359753 Compare October 15, 2021 04:50
@rodolforg
Copy link
Contributor Author

rodolforg commented Oct 15, 2021

Sorry - I was rebasing to master and accidentally included an additional commit.

I'll force push it again to fix it.

@rodolforg
Copy link
Contributor Author

done

@ice0 ice0 merged commit e0cdef3 into synfig:master Oct 15, 2021
@ice0
Copy link
Collaborator

ice0 commented Oct 15, 2021

Merged. Thank you!

@rodolforg rodolforg deleted the pseudo-random-cleanup branch October 25, 2021 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants