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

Return reference instead of non-null pointer #2637

Merged

Conversation

kimci86
Copy link
Contributor

@kimci86 kimci86 commented Aug 5, 2023

Since #2486 and #2494, Sprite and Text instances cannot hold a null pointer to the corresponding resource.
I suggest that getTexture and getFont return a reference instead of a pointer to make it clear that it is not null.
What do you think?

Note this is slightly changing Sprite constructor behavior. If I understand correctly, Sprite(texture, IntRect()) is equivalent to Sprite(texture) in master, whereas Sprite(texture, IntRect()) would actually create a Sprite with an empty texture rectangle with this PR. It looks to me this would be the correct behavior one would expect.

I also reviewed corresponding tests and fixed a small inconsistency in Text tests while at it.

The same kind of change would apply to Sound and getSoundBuffer once the default constructor gets removed as planned in #2507.

and do some simplification using the fact that a constructed sprite has
a non-null texture pointer.
because a the m_font pointer cannot be null.
@codecov
Copy link

codecov bot commented Aug 5, 2023

Codecov Report

Merging #2637 (371af2b) into master (954fc65) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #2637    +/-   ##
========================================
  Coverage   28.73%   28.73%            
========================================
  Files         229      229            
  Lines       19755    19753     -2     
  Branches     4725     4724     -1     
========================================
  Hits         5676     5676            
- Misses      13437    13584   +147     
+ Partials      642      493   -149     
Files Changed Coverage Δ
include/SFML/Graphics/Sprite.hpp 0.00% <ø> (ø)
include/SFML/Graphics/Text.hpp 0.00% <ø> (ø)
src/SFML/Graphics/Sprite.cpp 0.00% <0.00%> (ø)
src/SFML/Graphics/Text.cpp 0.00% <0.00%> (ø)

... and 23 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 954fc65...371af2b. Read the comment docs.

Copy link
Member

@ChrisThrasher ChrisThrasher left a comment

Choose a reason for hiding this comment

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

I really like this! It removes yet another potential source of bugs from the API and further reinforces the idea that SFML 3 is systematically eliminating invalid states from being representable in the type system.

@ChrisThrasher ChrisThrasher merged commit 669d9f5 into SFML:master Aug 5, 2023
89 checks passed
@kimci86 kimci86 deleted the feature/reference_for_non_null_pointer branch August 5, 2023 20:41
@ChrisThrasher
Copy link
Member

https://github.com/SFML/imgui-sfml/pull/250/files

Already found a real world example this is helpful. It's useful to know that a sprite always has a texture. It saves you some manual error checking.

@eXpl0it3r eXpl0it3r added this to the 3.0 milestone Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants