-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix several unused warnings #1791
Conversation
|
@binary1248 Thanks for the elaboration! The question is, should we deliberately fail if the provided |
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. |
53270fc
to
2c8ff29
Compare
Reverted the use of |
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. |
c400952
to
153b440
Compare
Btw. you don't need to fix everything in this PR. It's fine if you just pick the things you want. For the SoundWriters I implemented our own namespace
{
unsigned char toLower(unsigned char character)
{
return static_cast<unsigned char>(std::tolower(character));
}
} And for the #pragma warning(push, 0)
#include <windows.h>
#pragma warning(pop) |
Yes, as mentioned I won't push further for now, so feel free to add extra commits 🙂 |
b0a0b7b
to
c410cd6
Compare
84f69b4
to
1c03786
Compare
c410cd6
to
81d2002
Compare
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this cause a warning in the first place?
Also, is double division and then cast to float more precise than float division?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should test it for some very low and very high values.
Increased precision would be a nice side effect of this refactoring 🙂
|
||
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is strange. The negation needs static_cast
, but the logical or just below doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
a55605e
to
e23f030
Compare
e23f030
to
bc802c0
Compare
0, 0, 0, 0 | ||
) * std::pow(perlinFrequencyBase, -i); | ||
) * static_cast<float>(std::pow(perlinFrequencyBase, -i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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)) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean, a color()
function that accepts float
s, 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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I meant float
s.
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.
Is there anything left to do/review? |
Fix the errors? A red CI pipeline isn't exactly useful to anyone. |
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
7886fcd
to
77750f4
Compare
Didn't see/know that build logs expire. I've rebase the branch and pushed it again, so you'll see the latest failures. |
Superseded by #1846 |
Fixes part of #1785 (unused warnings).
Tasks
To be done
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.DefaultDepth(m_display, screen)
to what I think should bestatic_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.bitsPerPixel
parameter in the constructorGlxContext::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.