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

Fixed various type conversion/comparison warnings. #1325

Merged
merged 1 commit into from Jan 2, 2018

Conversation

eXpl0it3r
Copy link
Member

@eXpl0it3r eXpl0it3r commented Dec 7, 2017

I have applied some fixed for the various type conversion/comparison warnings Visual Studio has been complaining about for a while now.

I wasn't certain for every place if the change was done properly and for some places it certainly raises the question if the chosen type is good.

There's one remaining warning that we can't easily fix. It's in the network module when calling ::send(). Linux implements the socket send function with a size_t, while Windows uses int, causing a mismatch between platforms.

@@ -463,7 +463,7 @@ Ftp::Response Ftp::getResponse()
}

// Save the remaining data for the next time getResponse() is called
m_receiveBuffer.assign(buffer + in.tellg(), length - in.tellg());
m_receiveBuffer.assign(buffer + static_cast<std::streamoff>(in.tellg()), length - static_cast<std::size_t>(in.tellg()));
Copy link
Member

Choose a reason for hiding this comment

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

The first cast should not be needed.

Copy link
Member Author

@eXpl0it3r eXpl0it3r Dec 7, 2017

Choose a reason for hiding this comment

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

Visual Studio 2015 and 2017 32bits still complain about it.

According to cppreference std::streamoff is usually typedef-ed to long long and for the pointer manipulation you need an int thus there needs to be an explicit cast (for the architecture that uses 32bits ints).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry I though it was the same type (std::streamoff vs std::streampos). And why not std::size_t by the way?

Copy link
Member Author

Choose a reason for hiding this comment

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

I picked std::streamoff since that's what the compiler expected and converts to. I could try std::size_t as well, even though that will be unsigned int and not int, wouldn't it?

Copy link
Member

Choose a reason for hiding this comment

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

The first argument of assign is a const char*, which buffer + <any integral type> should produce no matter what the integral type is. So I don't get why we need a std::streamoff specifically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it to std::size_t. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

👍

@eXpl0it3r eXpl0it3r force-pushed the bugfix/compiler_type_warnings branch from 6f32826 to 73b43c2 Compare December 7, 2017 19:42
@eXpl0it3r eXpl0it3r added this to Review & Testing in SFML 2.5.0 Dec 7, 2017
@eXpl0it3r eXpl0it3r moved this from Review & Testing to Ready in SFML 2.5.0 Dec 8, 2017
@eXpl0it3r eXpl0it3r force-pushed the bugfix/compiler_type_warnings branch from 73b43c2 to 247b031 Compare January 2, 2018 19:01
@eXpl0it3r eXpl0it3r merged commit 247b031 into master Jan 2, 2018
@eXpl0it3r eXpl0it3r deleted the bugfix/compiler_type_warnings branch January 2, 2018 19:02
@eXpl0it3r eXpl0it3r moved this from Ready to Merged / Superseded in SFML 2.5.0 Jan 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.5.0
  
Merged / Superseded
Development

Successfully merging this pull request may close these issues.

None yet

3 participants