Fix several unused warnings #1791

Bromeon · 2021-04-14T20:38:50Z

Fixes part of #1785 (unused warnings).

Tasks

To be done

I don't know about the EglContext constructor (see issue discussion). We can remove it, but if we choose to keep it, someone else would need to help with the implementation. For those in the team, feel free to push directly to this branch.
I changed one argument DefaultDepth(m_display, screen) to what I think should be static_cast<int>(bitsPerPixel). I took this from the archaic X11 scrolls, if someone knows X11 better and would like to review and/or test, that would be nice.
I'm not sure what to do with the bitsPerPixel parameter in the constructor GlxContext::GlxContext(GlxContext* shared, const ContextSettings& settings, const WindowImpl* owner, unsigned int bitsPerPixel). I don't know if it makes sense to pass this in to an already existing window -- if it does, let me know. For now I commented the parameter out.

TLDR: review + testing on Unix needed.

binary1248 · 2021-04-15T00:23:22Z

DefaultDepth(m_display, screen) was purposely chosen because it minimizes (or even eliminates) the risk of the window not being able to be created because the requested bits per pixel is not provided by the X window system. GLX is the OpenGL extension to the X window system that provides surface properties via visuals. Unlike on Windows where the pixel format of the window is directly associated with the rendering surface, on Unix/Linux they are separate things. It sounds strange, but one can create a window with 8-bit depth and associate a 24-bit depth visual with it. The visual is the Unix equivalent of the pixel format on Windows and the available visuals will depend on GPU/driver support.

Bromeon · 2021-04-15T07:50:45Z

@binary1248 Thanks for the elaboration!

The question is, should we deliberately fail if the provided bitsPerPixel value doesn't work (there's no real way to signal failure from a constructor without exceptions), or as you suggest silently ignore the value and do our best to create the window nevertheless? In the latter case, we should probably document that this parameter has no meaning for X11.

binary1248 · 2021-04-15T10:58:26Z

Window creation should really never fail (and thankfully it almost never does unless there is something really broken with the environment). If any Xlib call fails, the "standard" procedure is that Xlib ends up terminating the whole process with some cryptic message written to the console as can be seen often enough. Xlib error handling is... a mess. We really don't want this to happen under any circumstance. In the off chance it happens that the best surface format that we chose for the user doesn't match what they expect, we already emit a warning. As such they shouldn't be surprised either way.

Bromeon · 2021-04-19T14:51:50Z

Reverted the use of bitsPerPixel on X11 and explicitly ignored parameters in EglContext constructor.

Bromeon · 2021-04-19T15:07:07Z

I think it may need a few more rounds 😬 I'll do one more cycle now, after that feel free to push other commits on this branch.

It's a bit annoying that CI immediately aborts, like this it's hard to see all warnings and they need to be corrected one by one.

eXpl0it3r · 2021-04-19T15:18:24Z

Btw. you don't need to fix everything in this PR. It's fine if you just pick the things you want.
I've also started looking at some of the warnings, but I'll wait until you're done with the things you want to fix.

For the SoundWriters I implemented our own toLower function, since std::tolower takes int which then doesn't work in combination with std::transform and std::string.

namespace
{
    unsigned char toLower(unsigned char character)
    {
        return static_cast<unsigned char>(std::tolower(character));
    }
}

And for the windows.h header I used

#pragma warning(push, 0)
#include <windows.h>
#pragma warning(pop)

Bromeon · 2021-04-19T16:20:34Z

Yes, as mentioned I won't push further for now, so feel free to add extra commits 🙂

Bromeon · 2021-04-21T06:34:23Z

src/SFML/System/Time.cpp

@@ -44,7 +44,7 @@ m_microseconds(0)
 ////////////////////////////////////////////////////////////
 float Time::asSeconds() const
 {
-    return m_microseconds / 1000000.f;
+    return static_cast<float>(static_cast<double>(m_microseconds) / 1000000.0);


Did this cause a warning in the first place?

Also, is double division and then cast to float more precise than float division?

Yes, because Uint64 and double have different precision if you look at the IEEE spec.

I'd assume so, if you have a very big integer and cast it to float, you will lose a lot of precision, while with a double, you'll have more precision.

But I'm open to suggestions, as I don't have too much experience with type conversions at this level. 😄

Maybe we should test it for some very low and very high values.
Increased precision would be a nice side effect of this refactoring 🙂

Bromeon · 2021-04-21T06:38:59Z

src/SFML/Window/EglContext.cpp


    eglCheck(result = eglGetConfigAttrib(m_display, m_config, EGL_SAMPLES, &tmp));

    if (result == EGL_FALSE)
        err() << "Failed to retrieve EGL_SAMPLES" << std::endl;

-    m_settings.antialiasingLevel = tmp;
+    m_settings.antialiasingLevel = static_cast<unsigned int>(tmp);


Not related to the change, but potential logic error:

If any of the eglGetConfigAttrib() calls fails, the result is still assigned to a m_settings member variable. I don't know what eglGetConfigAttrib() writes in such a case, but chances are that tmp is simply the last read value.

Bromeon · 2021-04-21T06:41:56Z

src/SFML/Window/Window.cpp

@@ -107,7 +107,7 @@ void Window::create(VideoMode mode, const String& title, Uint32 style, const Con
    // Check validity of style according to the underlying platform
    #if defined(SFML_SYSTEM_IOS) || defined(SFML_SYSTEM_ANDROID)
        if (style & Style::Fullscreen)
-            style &= ~Style::Titlebar;
+            style &= ~static_cast<Uint32>(Style::Titlebar);


This is strange. The negation needs static_cast, but the logical or just below doesn't?

Yeah, not sure why and I'm not even certain that this works correctly.
But you do have the different types of Uint32 for style and the enum being int, I assume by default.

Sounds to me like something we should align with C++17.

If you have a better way to do it let me know.

With C++17 we would probably use enum class, and then you'd need static_cast all the time for flag-like enums.
One neat idiom I've seen is overloading operator+:

// instead of static_cast<sf::Uint32>(Style::Titlebar) | static_cast<sf::Uint32>(Style::Resize) // you would have +Style::Titlebar | +Style::Resize

Or directly overload the operator for the enum values. There are also libraries like enum-flags trying to address this problem. But for now I think it's OK to just silence the warnings.

It's a bit a shame that static_cast is such a horrible syntactical choice for something occurring relatively often. It really makes code unncessarily verbose.

src/SFML/Window/WindowImpl.cpp

src/SFML/Network/TcpSocket.cpp

cmake/Macros.cmake

Bromeon · 2021-04-23T08:20:26Z

examples/island/Island.cpp

            0, 0, 0, 0
-        ) * std::pow(perlinFrequencyBase, -i);
+        ) * static_cast<float>(std::pow(perlinFrequencyBase, -i));


Why is this necessary?

perlinFrequencyBase is float, i is int.
pow(float, int) returns float (until C++11).

I see that https://en.cppreference.com/w/cpp/numeric/math/pow mentions that the return value would be promoted to double since C++11. How is that not a breaking change in the C++ standard library?!

Yeah, I was confused about this as well, but we don't really want to force people to use C++03 or disable warnings, so I guess while SFML isn't using C++11, we still should handle warnings for C++11, as long as it doesn't compromise our C++03 code base.

I guess it sort of is a breaking change in the standard?

Ah, so we're already running with C++11 settings, OK.
Yeah then the introduced change is OK (and I could live with it causing warnings on some older compilers).

Bromeon · 2021-04-23T08:27:22Z

examples/island/Island.cpp

        moisture < 0.6f ? sf::Color(0, 160, 0) :
-        moisture < 0.7f ? sf::Color(34 * (moisture - 0.6f) / 0.1f, 160 - 60 * (moisture - 0.6f) / 0.1f, 34 * (moisture - 0.6f) / 0.1f) :
+        moisture < 0.7f ? sf::Color(static_cast<sf::Uint8>(34 * (moisture - 0.6f) / 0.1f), 160 - static_cast<sf::Uint8>(60 * (moisture - 0.6f) / 0.1f), static_cast<sf::Uint8>(34 * (moisture - 0.6f) / 0.1f)) :


While eliminating warnings, this makes code totally unreadable, and misses the point of the example.

It's a bit a weakness of C++ that a lot of valid code gets warnings. In theory, something like short = short + short; could already be problematic. And it's not like static_cast actually solves a real issue, it just silences the compiler.

Should we maybe add a color() function to the example that accepts int?
Or a u8() function as a shortcut for static_cast<sf::Uint8>()?
Or any other idea?

You mean, a color() function that accepts floats, right? I guess, I can do that.
On the other hand it's an advanced example focusing on how to use Vulkan, so a few static_cast mixed in there, shouldn't really bother anyone...

Yes, I meant floats.

True, but someone might be interested in the noise algorithm, and for that the static_cast is just distraction...

Since auto-hinting is enabled, the advance will always be an integer number of pixels. The actual fractional advance is handled by bearings. #1827 (comment)

This reverts the change to addLine(), which no longer had its outline drawn after the offending commit.

Bromeon · 2021-11-02T18:37:17Z

Is there anything left to do/review?

eXpl0it3r · 2021-11-02T22:57:49Z

Fix the errors? A red CI pipeline isn't exactly useful to anyone.

Bromeon · 2021-11-02T23:03:25Z

Expired logs aren't, either 😉

Can you re-run CI?

- Convert where necessary - Adjust type where reasonable - Use SYSTEM headers for gl.h, stb* and vulkan

- Fix conversion & shadowing warnings - For the System & Window module

- Fix conversion and shadowing warnings - Add SYSTEM indicator for stb_*, FreeType and other headers

eXpl0it3r · 2021-11-02T23:38:40Z

Didn't see/know that build logs expire. I've rebase the branch and pushed it again, so you'll see the latest failures.
The unresolved review comments above also still need addressing.

eXpl0it3r · 2021-11-22T07:50:36Z

Superseded by #1846

eXpl0it3r changed the base branch from master to feature/compiler_warnings April 19, 2021 14:40

Bromeon force-pushed the bugfix/unused-warnings branch from 53270fc to 2c8ff29 Compare April 19, 2021 14:46

Bromeon changed the title ~~Fix several unused warnings; use bits per pixel in X11~~ Fix several unused warnings Apr 19, 2021

Bromeon marked this pull request as ready for review April 19, 2021 14:50

eXpl0it3r approved these changes Apr 19, 2021

View reviewed changes

eXpl0it3r added bug m:config s:accepted feature and removed bug labels Apr 19, 2021

eXpl0it3r approved these changes Apr 19, 2021

View reviewed changes

Bromeon force-pushed the bugfix/unused-warnings branch from c400952 to 153b440 Compare April 19, 2021 15:13

eXpl0it3r force-pushed the bugfix/unused-warnings branch 3 times, most recently from b0a0b7b to c410cd6 Compare April 20, 2021 14:28

binary1248 force-pushed the feature/compiler_warnings branch from 84f69b4 to 1c03786 Compare April 20, 2021 17:36

eXpl0it3r force-pushed the bugfix/unused-warnings branch from c410cd6 to 81d2002 Compare April 20, 2021 20:58

Bromeon commented Apr 21, 2021

View reviewed changes

eXpl0it3r force-pushed the bugfix/unused-warnings branch from a55605e to e23f030 Compare April 21, 2021 22:51

eXpl0it3r reviewed Apr 21, 2021

View reviewed changes

src/SFML/Network/TcpSocket.cpp Outdated Show resolved Hide resolved

eXpl0it3r force-pushed the bugfix/unused-warnings branch from e23f030 to bc802c0 Compare April 22, 2021 22:00

eXpl0it3r reviewed Apr 22, 2021

View reviewed changes

src/SFML/Network/TcpSocket.cpp Show resolved Hide resolved

Bromeon commented Apr 23, 2021

View reviewed changes

cschreib added 3 commits October 18, 2021 09:46

Fix incorrect glyph rect for text outline

4d2e5b1

Simplify glyph advance calculation

d950c93

Since auto-hinting is enabled, the advance will always be an integer number of pixels. The actual fractional advance is handled by bearings. #1827 (comment)

Partial revert from 40a0584

912422f

This reverts the change to addLine(), which no longer had its outline drawn after the offending commit.

binary1248 and others added 11 commits November 3, 2021 00:11

Enable compiler warnings.

3a8cf98

Fix several unused warnings

908cbae

Fix conversion and other warnings, mainly on Windows

9fe1b7f

Fix integer conversion warnings

0a1fae4

Fix type conversion warnings

6d7e7eb

Fix variable name shadowing warning

c024c49

Fix warnings in examples

aa75b80

- Convert where necessary - Adjust type where reasonable - Use SYSTEM headers for gl.h, stb* and vulkan

Fix conversion warnings for Unix

13aea48

- Fix conversion & shadowing warnings - For the System & Window module

Fix conversion warnings for the Network module

b03e1cb

Fix conversion warnings for the Graphics module

d0d2a67

- Fix conversion and shadowing warnings - Add SYSTEM indicator for stb_*, FreeType and other headers

Fix conversion warnings for the Audio module

77750f4

eXpl0it3r force-pushed the bugfix/unused-warnings branch from 7886fcd to 77750f4 Compare November 2, 2021 23:36

vittorioromeo mentioned this pull request Nov 19, 2021

Fix several unused warnings (continued) #1846

Merged

eXpl0it3r closed this Nov 22, 2021

eXpl0it3r added s:superseded and removed s:accepted labels Nov 22, 2021

eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Nov 30, 2021

eXpl0it3r added this to the 2.6 milestone Nov 30, 2021

eXpl0it3r moved this from Discussion to Done in SFML 2.6.0 Nov 30, 2021

dogunbound mentioned this pull request May 17, 2023

Pull request for 2.6.x CSFML SFML/CSFML#186

Merged

ChrisThrasher mentioned this pull request Jun 23, 2023

Implement remaining SFML 2.6.0 changes SFML/CSFML#187

Closed

ChrisThrasher deleted the bugfix/unused-warnings branch December 24, 2023 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix several unused warnings #1791

Fix several unused warnings #1791

Bromeon commented Apr 14, 2021 •

edited by eXpl0it3r

binary1248 commented Apr 15, 2021

Bromeon commented Apr 15, 2021

binary1248 commented Apr 15, 2021

Bromeon commented Apr 19, 2021

Bromeon commented Apr 19, 2021 •

edited

eXpl0it3r commented Apr 19, 2021

Bromeon commented Apr 19, 2021

Bromeon Apr 21, 2021

eXpl0it3r Apr 21, 2021

Bromeon Apr 21, 2021

Bromeon Apr 21, 2021

Bromeon Apr 21, 2021

eXpl0it3r Apr 21, 2021

Bromeon Apr 21, 2021

Bromeon Apr 23, 2021

eXpl0it3r Apr 23, 2021 •

edited

Bromeon Apr 26, 2021

Bromeon Apr 23, 2021

eXpl0it3r Apr 23, 2021

Bromeon Apr 23, 2021

Bromeon commented Nov 2, 2021

eXpl0it3r commented Nov 2, 2021

Bromeon commented Nov 2, 2021

eXpl0it3r commented Nov 2, 2021

eXpl0it3r commented Nov 22, 2021

Fix several unused warnings #1791

Fix several unused warnings #1791

Conversation

Bromeon commented Apr 14, 2021 • edited by eXpl0it3r

Tasks

To be done

binary1248 commented Apr 15, 2021

Bromeon commented Apr 15, 2021

binary1248 commented Apr 15, 2021

Bromeon commented Apr 19, 2021

Bromeon commented Apr 19, 2021 • edited

eXpl0it3r commented Apr 19, 2021

Bromeon commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eXpl0it3r Apr 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Nov 2, 2021

eXpl0it3r commented Nov 2, 2021

Bromeon commented Nov 2, 2021

eXpl0it3r commented Nov 2, 2021

eXpl0it3r commented Nov 22, 2021

Bromeon commented Apr 14, 2021 •

edited by eXpl0it3r

Bromeon commented Apr 19, 2021 •

edited

eXpl0it3r Apr 23, 2021 •

edited