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

Add support for SFML unity builds #1787

Closed

Conversation

vittorioromeo
Copy link
Member

@vittorioromeo vittorioromeo commented Apr 13, 2021

See #1788

@binary1248
Copy link
Member

In my opinion it is always nice for a library to support as many development styles as is technically feasible without having to compromise other factors such as cleanliness or runtime performance. I already contemplated such a change for quite a while since (as you discovered) the required changes are (or in theory should be) purely syntactical.

When browsing through the changeset I couldn't help but have the feeling that the name changes felt a bit too "C-like" to me. In C++ we have the wonderful namespace feature that was designed specifically to prevent name collisions. As far as I understand, nesting a named namespace within an unnamed namespace would inherit the internal linkage as well as provide the disambiguation necessary to support unity builds. In my opinion it would also make the code easier to read since only a TU-specific prefix would have to be prepended to the respective occurrences of the names within the TU.

Regarding the issue with glad: This is a tricky one. Basically the integration method that glad chose to use (which I actually like) is that of a "single-file header-only library" although being an OpenGL loader the single-file part is basically impossible to do in a clean way. Within an entire application, there should only be a single compiled instance of glad (or to be more exact a single compiled instance of each of its files). As such, to keep supporting non-unity builds we can't define SF_GLAD_GL_IMPLEMENTATION in multiple places, that will just break everything when building as a static library as CI shows. I still don't fully understand the necessity to define SF_GLAD_GL_IMPLEMENTATION multiple times in this changeset. The concrete definition of its symbols is really only required in the graphics module (access to e.g. only the #defines doesn't require compilation), and whether building SFML in the traditional way or in a unity build, this shouldn't change.

I support this PR, but the current changes have to be worked on a bit more.

@vittorioromeo
Copy link
Member Author

@binary1248: thanks for the feedback! I will wait for more maintainters' opinions, and if everyone is on-board I will clean it up and will try to figure out how to deal with glad.

Would modifying/splitting the glad header be reasonable at all? Or do we want to keep it untouched?

@binary1248
Copy link
Member

We already have a reported regression in #1786 because I updated glad and forgot to add in the tweak that I did by hand in the previous version. As such, going forward, I really prefer not having to modify glad in any way from its auto-generated form.

@vittorioromeo
Copy link
Member Author

@binary1248: I am starting this from scratch using namespaces. This is the error I get when trying to build sfml-graphics with a unity build:

In file included from C:\OHWorkspace\SFML\build\src\SFML\Graphics\CMakeFiles\sfml-graphics.dir\Unity\unity_0_cxx.cxx:13:
C:/OHWorkspace/SFML/src/SFML/Graphics/GLExtensions.cpp: In function 'void sf::priv::ensureExtensionsInit()':
C:/OHWorkspace/SFML/src/SFML/Graphics/GLExtensions.cpp:57:9: error: 'gladLoadGL' was not declared in this scope
   57 |         gladLoadGL(reinterpret_cast<GLADloadfunc>(sf::Context::getFunction));
      |         ^~~~~~~~~~

How am I supposed to make that declaration visible without making everything explode?

It's kinda weird. GLExtensions.cpp has:

#define SF_GLAD_GL_IMPLEMENTATION
#include <SFML/Graphics/GLExtensions.hpp>

And it relies on the transitive include of GLExtensions.hpp:

#ifndef SFML_GLEXTENSIONS_HPP
#define SFML_GLEXTENSIONS_HPP

////////////////////////////////////////////////////////////
// Headers
////////////////////////////////////////////////////////////
#include <SFML/Config.hpp>
#include <glad/gl.h>

// ...

What is the correct way of using <glad/gl.h> and the SF_GLAD_GL_IMPLEMENTATION macro in this case?

@vittorioromeo
Copy link
Member Author

Better version in #1788

@eXpl0it3r eXpl0it3r added this to the 2.6 milestone Oct 18, 2021
@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Oct 18, 2021
@eXpl0it3r eXpl0it3r moved this from Discussion to Done in SFML 2.6.0 Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants