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

Std explicit #62

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

tgfrerer
Copy link
Contributor

@tgfrerer tgfrerer commented Sep 8, 2017

use explicit std, in header files, using namespace std in .cpp files. This is necessary for latest openFrameworks, which (finally) has more stricter handling of namespaces.

  • also inlines glsl for engineVk

tgfrerer and others added 5 commits July 14, 2017 15:38
+ use inline glsl instead of external shader files to render imgui
+ use consistent string literal marker for inlined glsl source code.
@@ -29,7 +29,7 @@ namespace ofxImGui
virtual void onKeyReleased(ofKeyEventArgs& event) = 0;
virtual void onWindowResized(ofResizeEventArgs& window);

virtual GLuint loadTextureImage2D(unsigned char * pixels, int width, int height);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've changed this here, as the base engine should be agnostic of its final rendering backend - and GLuint inserts a dependency to OpenGL.

@jvcleave
Copy link
Owner

jvcleave commented Sep 8, 2017

What's the benefit of explicit std?

@tgfrerer
Copy link
Contributor Author

tgfrerer commented Sep 8, 2017

It looks like openFrameworks > 0.10 will be a bit more picky about namespaces.

... it allows us to keep the global namespace a bit cleaner - oF before 0.10, inofConstants.h, did import the full std namespace into the global namespace, which is considered bad practice.

The strongest itch comes from having this using directive in a header file - and because headers include each other, and other headers, you could never be sure about the state of the global namespace if you included an oF header...

Note that it's perfectly ok to do using namespace std in a .cpp file, because this using directive is only valid for the current "compilation unit", i.e. the current .cpp file, and does not "infect" any other files.

The namespace drive is part of a more general drive of disentangling the oF headers, and streamlining dependencies, all yak shaving operations to ultimately reduce complexity and compile times :)

@jvcleave
Copy link
Owner

jvcleave commented Sep 8, 2017

I'm super skeptical about modern cpp practices and the promises it brings. I tend to agree with this comment

I guess it's best to fall in line with what our dependency is doing. You seem to be working on this more than I am so I've invited you as a collaborator - if you want just join and merge whenever you see fit

@tgfrerer
Copy link
Contributor Author

tgfrerer commented Sep 8, 2017

Oh, it's not a modern cpp practice - using namespace xy in header files has been discouraged for over 17 years now ;)

The way i read the comment you linked, it seems to approve of not using using namespace std in header files.

I believe openFrameworks had the line using namespace std in ofConstants.h because initially, it made things easier, and for folks coming from other languages, which don't use namespaces, the code looked friendlier.

But oF has grown, and there are many libraries that somehow interact with it, and the more oF makes sure to keep the global namespace tidy, the less unintended side-effects we might run into in the future. Debugging header-dependency- and namespace collisions is really tough...

@jvcleave
Copy link
Owner

jvcleave commented Sep 8, 2017

I wish 17 years seemed like a long time ago.

It's entirely possible I don't fully understand the issue and will look at some of the links. (I've only had a conflict once with ctime.h which clashed with boost).

@valillon
Copy link

Hi guys,

I tried to compile ofxImGui against the current OF master branch and experienced those namespace errors that weren't in OF 0.9.8.

Not sure it's due to pre-release or simply because as @tgfrerer mentioned OF has embraced that safer practice. In any case, adding std:: make it compile cleanly.

@Daandelange Daandelange added the legacy-version For issues releated to the older legacy code, before all the refactorings in the develop branch. label Jul 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-version For issues releated to the older legacy code, before all the refactorings in the develop branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants