Minor warnings to fix (-Wunused-parameter, -Wsign-compare)

[vittorioromeo]

Subject of the issue

There are some warnings produced when compiling SFML by source. I've looked at the code and they seem benign -- they just need to be suppressed.

Your environment

• Windows 10, MSYS2 + MinGW
• SFML git master
• clang++ 10.x
• -Wall -Wextra -Wpedantic

Steps to reproduce

Build.

---

src/SFML/Window/EglContext.cpp: In constructor ‘sf::priv::EglContext::EglContext(sf::priv::EglContext*, const sf::ContextSettings&, unsigned int, unsigned int)’:
src/SFML/Window/EglContext.cpp:163:36: warning: unused parameter ‘shared’ [-Wunused-parameter]
  163 | EglContext::EglContext(EglContext* shared, const ContextSettings& settings, unsigned int width, unsigned int height) :
      |                        ~~~~~~~~~~~~^~~~~~
src/SFML/Window/EglContext.cpp:163:67: warning: unused parameter ‘settings’ [-Wunused-parameter]
  163 | EglContext::EglContext(EglContext* shared, const ContextSettings& settings, unsigned int width, unsigned int height) :
      |                                            ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
src/SFML/Window/EglContext.cpp:163:90: warning: unused parameter ‘width’ [-Wunused-parameter]
  163 | EglContext::EglContext(EglContext* shared, const ContextSettings& settings, unsigned int width, unsigned int height) :
      |                                                                             ~~~~~~~~~~~~~^~~~~
src/SFML/Window/EglContext.cpp:163:110: warning: unused parameter ‘height’ [-Wunused-parameter]
  163 | EglContext::EglContext(EglContext* shared, const ContextSettings& settings, unsigned int width, unsigned int height) :
      |                                                                                                 ~~~~~~~~~~~~~^~~~~~



src/SFML/Window/Unix/GlxContext.cpp: In constructor ‘sf::priv::GlxContext::GlxContext(sf::priv::GlxContext*, const sf::ContextSettings&, const sf::priv::WindowImpl*, unsigned int)’:
src/SFML/Window/Unix/GlxContext.cpp:128:115: warning: unused parameter ‘bitsPerPixel’ [-Wunused-parameter]
  128 | GlxContext::GlxContext(GlxContext* shared, const ContextSettings& settings, const WindowImpl* owner, unsigned int bitsPerPixel) :



src/SFML/Window/Unix/WindowImplX11.cpp: In member function ‘void sf::priv::WindowImplX11::setVideoMode(const sf::VideoMode&)’:
src/SFML/Window/Unix/WindowImplX11.cpp:1338:33: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
 1338 |         if (res->modes[i].width == static_cast<int>(mode.width) &&
      |             ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/SFML/Window/Unix/WindowImplX11.cpp:1339:34: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
 1339 |             res->modes[i].height == static_cast<int>(mode.height))
      |             ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~



src/SFML/Graphics/RenderTextureImplFBO.cpp: In function ‘void {anonymous}::contextDestroyCallback(void*)’:
src/SFML/Graphics/RenderTextureImplFBO.cpp:80:39: warning: unused parameter ‘arg’ [-Wunused-parameter]
   80 |     void contextDestroyCallback(void* arg)
      |                                 ~~~~~~^~~

[eXpl0it3r]

Don't think we should ignore these errors

EglContext

SFML/src/SFML/Window/EglContext.cpp

Lines 163 to 170 in 59df9d0

	EglContext::EglContext(EglContext* shared, const ContextSettings& settings, unsigned int width, unsigned int height) :
	m_display (EGL_NO_DISPLAY),
	m_context (EGL_NO_CONTEXT),
	m_surface (EGL_NO_SURFACE),
	m_config (NULL)
	{
	ensureInit();
	}

Why are we not using the provided parameters? 🤔

WindowImplX11

Doesn't really cost us anything to convert to unsigned int then again SFML video mode is already unsigned int, so maybe the conversion is not needed (anymore).

RenderTextureImplFBO

SFML/src/SFML/Graphics/RenderTextureImplFBO.cpp

Lines 80 to 106 in 498d7ee

	void contextDestroyCallback(void* arg)
	{
	sf::Lock lock(mutex);
	sf::Uint64 contextId = sf::Context::getActiveContextId();
	// Destroy active frame buffer objects
	for (std::set<std::map<sf::Uint64, unsigned int>*>::iterator frameBuffersIter = frameBuffers.begin(); frameBuffersIter != frameBuffers.end(); ++frameBuffersIter)
	{
	for (std::map<sf::Uint64, unsigned int>::iterator iter = (*frameBuffersIter)->begin(); iter != (*frameBuffersIter)->end(); ++iter)
	{
	if (iter->first == contextId)
	{
	GLuint frameBuffer = static_cast<GLuint>(iter->second);
	glCheck(GLEXT_glDeleteFramebuffers(1, &frameBuffer));
	// Erase the entry from the RenderTextureImplFBO's map
	(*frameBuffersIter)->erase(iter);
	break;
	}
	}
	}
	// Destroy stale frame buffer objects
	destroyStaleFBOs();
	}

I guess if we don't use arg we could also just use void contextDestroyCallback(void*) which should get rid of the warning.

If anyone wants to provide PRs for these, I'm happy to accept them.

[elijahfhopp]

I would also like to note that there is a unused on the Glx context as well:

SFML/src/SFML/Window/Unix/GlxContext.cpp:128:115: warning: unused parameter ‘bitsPerPixel’ [-Wunused-parameter]
  128 | GlxContext::GlxContext(GlxContext* shared, const ContextSettings& settings, const WindowImpl* owner, unsigned int bitsPerPixel) :
      |    

(Note: I am running GCC 10.1.0, not clang)

[Bromeon]

EglContext

Don't think we should ignore these errors

SFML/src/SFML/Window/EglContext.cpp

Lines 163 to 170 in 59df9d0

	EglContext::EglContext(EglContext* shared, const ContextSettings& settings, unsigned int width, unsigned int height) :
	m_display (EGL_NO_DISPLAY),
	m_context (EGL_NO_CONTEXT),
	m_surface (EGL_NO_SURFACE),
	m_config (NULL)
	{
	ensureInit();
	}

Why are we not using the provided parameters? 🤔

Good question, but it is like this since the initial addition of the file 8 years ago. It looks like we simply never implemented that constructor, and somehow no one bothered using it 🙂

We have multiple options:

• Remove it -- technically a breaking change, but it's already broken, better do it at compile time than runtime.
• Implement it, might require @binary1248's or someone else's help.

WindowImplX11

In WindowImplX11, the conversion is not needed, the X11 fields are already unsigned int.

Pull request

I started a draft PR (see link above this comment) to address some of the issues, but would need feedback to continue with the rest.

[binary1248]

GlContext.cpp forwards all context management functionality to the concrete platform implementations. If there are certain unused parameters on one implementation they will certainly be used by some other. As such, I don't think it makes sense to discuss cutting down on the overloads until it can be shown a certain overload isn't used by any implementation. If the aim is to reduce the warnings then I don't see why we can't just remove the parameter names.

[Bromeon]

I see. For the constructor overloads not implemented, I could at least add a sf::err() message, since that's SFML's current way of signaling errors.

[binary1248]

That would make sense.

[eXpl0it3r]

See #1792 but not sure if -Wsign-conversion is more strict than -Wsign-compare

[binary1248]

See #1792 but not sure if -Wsign-conversion is more strict than -Wsign-compare

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

-Wsign-compare

    Warn when a comparison between signed and unsigned values could produce an incorrect result when the signed value is converted to unsigned. In C++, this warning is also enabled by -Wall. In C, it is also enabled by -Wextra.

-Wsign-compare is enabled with -Wall, so #1792 will have both enabled.

[eXpl0it3r]

Implemented with #1846