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

Rename all Effect classes to ZynEffect classes #16

Closed
wants to merge 2 commits into from

Conversation

JohannesLorenz
Copy link
Contributor

I suddenly got tons of different segfaults with old zyn in master. Looking at gdb or gcc's address sanitizer, both say that ~Chorus() of zyn calls ~Effect() of LMMS (In Zyn, Effect is a base class of Chorus, but it's not the LMMS Effect class). Reason might be a new compiler or whatever.

This is a stupid fix I made solely with sed -i. It's not perfect, a few classes too much are replaced, but it works and this submodule here is only temporary until we have Lv2.

Note: minor changes in the LMMS main repo's CMakeLists are required (changing the file names there).

No testing or code/style review required. But I'd like someone to confirm that the commit makes sense.

Thanks on advance.

@JohannesLorenz
Copy link
Contributor Author

@tresf I hope it's OK I ask you. It should not take more than a few minutes.

@tresf
Copy link
Member

tresf commented Dec 29, 2019

Hmm... another namespace problem?

Won't this make future fast-forwarding code a complete mess? Perhaps we run sed at compile time to fix the namespace problems?

Furthemore, many lines are renaming comments, which represent a bunch of touched lines that are not needed (nor intuitively phrased now).

Lastly, if there's a namespace problem with Effect, shouldn't we fix it LMMS-side, like we did with Engine ?

@JohannesLorenz
Copy link
Contributor Author

Hmm... another namespace problem?

Won't this make future fast-forwarding code a complete mess? Perhaps we run sed at compile time to fix the namespace problems?

That won't work with MSVC builds. But it's also more than one sed call, and a few file renames.

Lastly, if there's a namespace problem with Effect, shouldn't we fix it LMMS-side, like we did with Engine ?

I feel like the time has come for giving LMMS an lmms namespace. I find that much nicer than renaming Effect to something like LmmsEffect.

Right now, I have a completely different idea: The zyn namespaces have been added in 3.0.2. Maybe we can update our zyn submodule (by merging from zyn's master)?

@DomClark
Copy link
Member

I agree with using namespaces to address this - this sort of situation is the entire point of having them in the language.

The zyn namespaces have been added in 3.0.2. Maybe we can update our zyn submodule (by merging from zyn's master)?

Sounds good, and this is probably something we want to do eventually anyway, I guess. How does this relate to Zyn-Fusion?

@JohannesLorenz
Copy link
Contributor Author

Since we will soon have a namespace lmms, I'll close my own PR. The namespace solution is much cleaner.

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.

3 participants