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

Remove 'using namespace' from include files #204

Closed
schauveau opened this issue Mar 18, 2019 · 3 comments · Fixed by #296
Closed

Remove 'using namespace' from include files #204

schauveau opened this issue Mar 18, 2019 · 3 comments · Fixed by #296
Labels
code Source code cleanup, streamlining, or style tweaks enhancement

Comments

@schauveau
Copy link

The libopenshot header files contain multiple using namespace statements for openshot and std. This is a bad design for several reasons that I will not enumerate here.
Look for instance in https://stackoverflow.com/questions/5849457/using-namespace-in-c-headers and https://www.acodersjourney.com/top-10-c-header-file-mistakes-and-how-to-fix-them/

@ferdnyc
Copy link
Contributor

ferdnyc commented Apr 7, 2019

Just for the record, I spent a very, very long night taking a look at this issue and, well... the results were less than satisfying. I did manage to get all of the using namespace directives out of the headers, but the only way I could get the SWIG wrappers to compile was to stick using namespace openshot; into the openshot.i files instead, and even then the wrappers didn't come out right. (I have no idea if the library even came out right, TBH.)

The results of my efforts are in my namespace-cleanup branch if anyone's interested. What's there does compile, at least for me. But If you try to run OpenShot with it, it'll die right at startup as soon as it tries to create the Timeline object. It doesn't understand ChannelLayout, which comes from the include/ChannelLayouts.h file.

ChannelLayout is an enum stuffed full of strings that FFmpeg #defines. Those in turn come from include/FFmpegUtilities.h which doesn't have a namespace openshot {} wrapped around it, and it breaks if I add one. I think it's because the FFmpeg headers are included under an extern "C". Something about that seems to be throwing SWIG off its game, now that everything isn't polluted into all of the wrong namespaces, and I'm not sure how to fix it.

@ferdnyc
Copy link
Contributor

ferdnyc commented Aug 5, 2019

I took another run at this, and by jove I think I've got it!

Still testing, but libopenshot compiled and passed its tests, and I ran OpenShot against the corresponding openshot.py / _openshot.so. So far everything seems to be in order. Despite there being not a single using namespace … statement anywhere in the headers, or in SWIG's openshot.i files.

@ferdnyc
Copy link
Contributor

ferdnyc commented Aug 5, 2019

My std-prefixes branch contains the results of that effort, though once I'm more confident that it's really OK I plan to submit it as a PR, at which point I'll link it to this issue.

@ferdnyc ferdnyc added enhancement code Source code cleanup, streamlining, or style tweaks labels Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Source code cleanup, streamlining, or style tweaks enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants