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

Replaced unsigned int with std::size_t for array indices/sizes #739

Merged
merged 1 commit into from Dec 28, 2014

Conversation

Projects
None yet
4 participants
@Bromeon
Member

Bromeon commented Nov 17, 2014

See http://en.sfml-dev.org/forums/index.php?topic=16747

The type unsigned int is replaced by std::size_t in places where values clearly represent array indices or sizes. This affects mainly the sf::VertexArray class and the sf::Shape class hierarchy. The change makes the API more consistent to existing parts like sf::String.

Points to consider regarding the decision on introducing this PR into SFML 2.2:

  • It is an API change, although a backwards-compatible one. This means the documentation will be updated, in particular the one on the website will look different when this modification is merged. Thus, if this is merged shortly after 2.2, people will have an outdated online doc until the next release (although it will hardly make a real difference whether one reads unsigned int or std::size_t).
  • It is not a critical bugfix. Currently, compiler warnings are generated on systems where unsigned int and std::size_t are different types.
  • The change is cosmetical and should not have noticeable implications on the behavior. In practice, an array of more than 4G vertices or points (where the size would not fit into unsigned int) would lead to completely different problems.

I would appreciate if the latest changes could be tested with fully enabled conversion warnings -- in my environment, unsigned int and std::size_t are the same types, so I don`t see potential warnings.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Nov 17, 2014

Member

Compiling master with -WEverything gives me a verrry long list of warnings, of course. Here is the list of warnings types. I don't have time right now to go through all warnings so I uploaded the log when I compile this branch with -Wconversion -Wcovered-switch-default -Wdocumentation -Wno-documentation-unknown-command -Wfloat-equal -Wmissing-field-initializers -Wmissing-variable-declarations -Wpedantic -Wswitch-enum -Wno-vla-extension -Wno-old-style-cast -Wno-c++11-long-long. You can find it here (too big for gist I think). Sorry I couldn't trim it to only relevant warnings.. :-/

Member

mantognini commented Nov 17, 2014

Compiling master with -WEverything gives me a verrry long list of warnings, of course. Here is the list of warnings types. I don't have time right now to go through all warnings so I uploaded the log when I compile this branch with -Wconversion -Wcovered-switch-default -Wdocumentation -Wno-documentation-unknown-command -Wfloat-equal -Wmissing-field-initializers -Wmissing-variable-declarations -Wpedantic -Wswitch-enum -Wno-vla-extension -Wno-old-style-cast -Wno-c++11-long-long. You can find it here (too big for gist I think). Sorry I couldn't trim it to only relevant warnings.. :-/

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Nov 18, 2014

Member

That is a ton of warnings 😨

Most of them are not about std::size_t <-> unsigned int, I'd say we focus on them here. Among those, many I found are in external libraries, e.g.

  • src/SFML/Window/OSX/cg_sf_conversion.cpp
  • src/SFML/Graphics/stb_image/stb_image.h

I suggest we address the warnings in a separate modification (after SFML 2.2). However, we can't fix the ones in external libraries, and even among those in SFML there are loads of useless ones, e.g.

std::size_t rowSize = m_size.x * 4;
m_pixels.end() - rowSize // warning: conversion size_t -> ptrdiff_t

Fixing every single one of them would bloat the code with static_cast and make everything unreadable...

Member

Bromeon commented Nov 18, 2014

That is a ton of warnings 😨

Most of them are not about std::size_t <-> unsigned int, I'd say we focus on them here. Among those, many I found are in external libraries, e.g.

  • src/SFML/Window/OSX/cg_sf_conversion.cpp
  • src/SFML/Graphics/stb_image/stb_image.h

I suggest we address the warnings in a separate modification (after SFML 2.2). However, we can't fix the ones in external libraries, and even among those in SFML there are loads of useless ones, e.g.

std::size_t rowSize = m_size.x * 4;
m_pixels.end() - rowSize // warning: conversion size_t -> ptrdiff_t

Fixing every single one of them would bloat the code with static_cast and make everything unreadable...

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 22, 2014

Member

2.3 sounds fine to me. Though should we fix (some) warnings in this branch or open up a new one?

Member

eXpl0it3r commented Dec 22, 2014

2.3 sounds fine to me. Though should we fix (some) warnings in this branch or open up a new one?

@eXpl0it3r eXpl0it3r added this to the 2.3 milestone Dec 22, 2014

@eXpl0it3r eXpl0it3r removed the s:unassigned label Dec 22, 2014

@eXpl0it3r eXpl0it3r self-assigned this Dec 22, 2014

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 22, 2014

Member

I suggest we stick to size_t here since it's already quite some work.

Member

mantognini commented Dec 22, 2014

I suggest we stick to size_t here since it's already quite some work.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 22, 2014

Member

@Bromeon Would be nice if you could do a rebase.

Everyone else: Review and test!

Member

eXpl0it3r commented Dec 22, 2014

@Bromeon Would be nice if you could do a rebase.

Everyone else: Review and test!

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 23, 2014

Member

Reviewed 👍

Member

LaurentGomila commented Dec 23, 2014

Reviewed 👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 26, 2014

Member

There seem to be no possible merge conflicts, as such I can also merge this without @Bromeon rebasing it.

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Dec 26, 2014

There seem to be no possible merge conflicts, as such I can also merge this without @Bromeon rebasing it.

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r merged commit 1cfa5c6 into master Dec 28, 2014

@eXpl0it3r eXpl0it3r deleted the bugfix/size_t branch Dec 28, 2014

@zsbzsb zsbzsb referenced this pull request May 1, 2015

Closed

Track releasing 2.3 #70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment