Skip to content

Conversation

@irrenhaus3
Copy link
Contributor

Removes "using namespace std" from global scope, which is a common source of symbol clashes. Adds std:: prefix to affected symbols in the header and some sources which included it transitively.

@LmmsBot
Copy link

LmmsBot commented Jul 12, 2021

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Windows

Linux

macOS

🤖
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://14354-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.129%2Bg96dc204eb-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14354?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://14353-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.129%2Bg96dc204eb-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14353?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/aytw7brepcybvt8m/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/40057830"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/eg99bml2nomxtrop/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/40057830"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://14355-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.129%2Bg96dc204-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14355?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://14356-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.129%2Bg96dc204eb-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14356?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "1d72ec7bed0c3e945153de20d977824f39eb0045"}

@Veratil
Copy link
Contributor

Veratil commented Jul 13, 2021

LGTM, double check for any other locations!

@PhysSong
Copy link
Member

Isn't it possible to add using std::foo;s in place of using namespace std; instead of adding std:: everywhere we need?

@rdrpenguin04
Copy link
Contributor

That can lead to just as many name conflicts, with the only difference being us opting into every one individually.

The people here have many things to say about this: StackOverflow link. While this often doesn't address using std::*, making use of any using declaration at header scope is nearly universally a bad idea for the same reasons. At implementation scope, it's fine, but it's generally clearer to specifically name when something is coming from std and when we have our own implementation.

@irrenhaus3
Copy link
Contributor Author

That can lead to just as many name conflicts, with the only difference being us opting into every one individually.

The people here have many things to say about this: StackOverflow link. While this often doesn't address using std::*, making use of any using declaration at header scope is nearly universally a bad idea for the same reasons. At implementation scope, it's fine, but it's generally clearer to specifically name when something is coming from std and when we have our own implementation.

Agreed. using [namespace]::[symbol] is OK inside a lexical scope of some function or class, but at global scope it's more trouble than it's worth. IMO just writing std::foo instead of foo isn't too big a hassle either, so my personal usage of using std::foo is pretty low anyways - but that's more of a matter of personal style.

As for other locations, using namspace std; appears at global scope in the following header files:

3rdparty/jack2:

  • common/JackNetTool.h
  • macosx/coreaudio/JackCoreAudioAdapter.h
  • macosx/coreaudio/JackCoreAudioDriver.h

plugins:

  • zynaddsubfx/src/Effects/Alienwah.h
  • zynaddsubfx/src/Tests/[several files]
  • carlabase/carla/source/native-plugins/external/zynaddsubfx/UI/TipWin.h
  • LadspaEffect/calf/veal/src/calf/orfanidis_eq.h

Most of these occurences might be contained to their respective projects - at least the Zyn Test header instances definitely are.

@JohannesLorenz
Copy link
Contributor

plugins:

* zynaddsubfx/src/Effects/Alienwah.h
* zynaddsubfx/src/Tests/[several files]

We could fix the Alienwah in our fork (though note that this has been fixed on zyn's upstream). The Test files are only a source for auto-generating cpp files using "cxxtest", and they will only be ever included by their autogenerated cpp files. So I'm OK with leaving those as they are.

This PR LGTM, but feel free to fix the occurences in the submodules mentioned - at least in those which we own (e.g. LMMS/zynaddsubfx).

@PhysSong
Copy link
Member

Note: The changes should be tested against GCC 6 to make sure it doesn't break builds. See #5831 for an example.

@JohannesLorenz
Copy link
Contributor

@PhysSong How do we test GCC6? Is this part of our CI, or must we ask one of our devs who has GCC6? I think @Veratil posted with GCC6 on discord?

@irrenhaus3 irrenhaus3 changed the title Remove 'using namespace std;' from lmms_math.h Remove 'using namespace std;' from LMMS headers Jul 17, 2021
@irrenhaus3
Copy link
Contributor Author

Update on veal: orfanides_eq.h does in fact not contain a using namespace std anymore, I was mistaken because my submodule was not up to date. Fun!
So with the Zyn patch, this removes using namespace std from global scope in all LMMS-owned header files except the self-contained Zyn tests.
Do not merge before the PR in the Zyn repo has been merged and the temp. URL for the submodule has been reverted.

@JohannesLorenz
Copy link
Contributor

@irrenhaus3 I merged the zyn submodule to its master. If you revert the remote, I think we are ready to go.

@JohannesLorenz JohannesLorenz self-requested a review July 17, 2021 17:01
@irrenhaus3 irrenhaus3 mentioned this pull request Jul 18, 2021
@JohannesLorenz JohannesLorenz self-requested a review July 23, 2021 17:12
@JohannesLorenz JohannesLorenz merged commit 0abbd6c into LMMS:master Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants