Remove bugprone `sf::Text` constructor by ChrisThrasher · Pull Request #2486 · SFML/SFML

ChrisThrasher · 2023-04-02T23:09:57Z

Description

The default constructor for sf::Text made it possible to accidentally forget to assign a font. Removing that default constructor means users are forced to supply a font making sf::Text harder to misuse. While I was at it, I also tweaked the existing constructor to allow users to specify a font without a default string to avoid the needless "" argument.

This required some changes to the Joystick example which made heavy use of default constructed text objects. The modifications to that example prove that a default constructor is not required to effectively use sf::Text.

codecov · 2023-04-02T23:12:59Z

Codecov Report

Merging #2486 (a3c678c) into master (f371a99) will decrease coverage by 5.54%.
The diff coverage is 0.00%.

❗ Current head a3c678c differs from pull request most recent head 221eb2a. Consider uploading reports for the commit 221eb2a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2486      +/-   ##
==========================================
- Coverage   29.14%   23.60%   -5.54%     
==========================================
  Files         187      212      +25     
  Lines       15610    18658    +3048     
  Branches     3789     4457     +668     
==========================================
- Hits         4549     4405     -144     
- Misses      10667    13758    +3091     
- Partials      394      495     +101

Impacted Files	Coverage Δ
include/SFML/Graphics/RenderWindow.hpp	`0.00% <ø> (-50.00%)`	⬇️
include/SFML/Graphics/Text.hpp	`0.00% <ø> (ø)`
src/SFML/Graphics/Text.cpp	`0.00% <0.00%> (ø)`

... and 81 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 f371a99...221eb2a. Read the comment docs.

Bromeon

Repeating my comment from here:

A case we need to think about:
sf::Text text(font);
addText(std::move(text)); // give ownership away

// now `text` is like a "default-constructed" instance -- what are its semantics?
Also, sf::Font relates to sf::Text the same way that sf::Texture relates to sf::Sprite, or sf::SoundBuffer to sf::Sound. What do you think about those resources?

This will force users to use std::optional<sf::Text> in classes that use text that cannot be immediately initialized -- which is probably a good thing to highlight nullability. But we might need to check user feedback here.

On a tangential note, in retrospective I think removing the default font was a user-unfriendly decision. Sure, it keeps the binary smaller, but it's quite a bit of extra effort to get some simple text or debug info displayed. It's probably in line though with seeing SFML as a more mid-level media abstraction, rather than a "batteries included" sort of game engine.

ChrisThrasher · 2023-04-03T17:10:09Z

On a tangential note, in retrospective I think removing the default font was a user-unfriendly decision.

I'm also open to the idea of bringing back a default font should someone choose to lead that effort. It's very user-friendly to not require everyone load their own font. It's in line with how we vendor all dependencies instead of making users supply them.

ChrisThrasher · 2023-04-03T17:30:22Z

Regarding moved-from types, that's a good point. We define sf::Sound to nullify the underlying SoundBuffer when copied which means it's valid for that class to hold a null pointer. It's not unreasonable to want a moved-from Text to have no font associated with it. If it's possible for a given Text to have its font removed from it, that implies that maybe it's also valid to allow for default construction. I think this ultimately comes down to what semantics we want for moving/copying Text. As it stands now, it's reasonable to have no default constructor but if we change its copy or move semantics in the future then we may want to put it back.

Bromeon · 2023-04-03T21:26:39Z

If it's possible for a given Text to have its font removed from it, that implies that maybe it's also valid to allow for default construction.

Exactly. We can basically define the "moved-from" state in two ways:

Safe to destroy, but don't touch anymore. That instance is poisoned.
Defined semantics, can be re-initialized.

I'm also totally OK when going with (1) -- in that case we wouldn't need a default constructor, as it's not part of the public API to reuse "empty" sf::Text instances. Are there ways to generate compiler/linter warnings upon use of moved-from custom classes?

ChrisThrasher · 2023-04-03T23:08:32Z

Are there ways to generate compiler/linter warnings upon use of moved-from custom classes?

I don't believe we can make any changes to the library to make such bugs easier to detect. It's squarely on users to not use moved-from objects.

Exactly. We can basically define the "moved-from" state in two ways:

Safe to destroy, but don't touch anymore. That instance is poisoned.

Defined semantics, can be re-initialized.

I'm inclined to stick to (1). As far as I'm aware, our intent with adding more move semantics is to elide copies for the sake of performance. It's worth bearing in mind that if we start with option (1), we can always make moved-from objects more defined in the future, but if we promise that a given moved-from object is safe to use without reinitializing, we can't rescind that promise without breaking the API. (1) is the lazy option that gives us more leeway going forward.

vittorioromeo · 2023-04-04T14:07:41Z

I'm generally in favour of this change.

Safe to destroy, but don't touch anymore. That instance is poisoned.

I'm also in favour of this approach.

Are there ways to generate compiler/linter warnings upon use of moved-from custom classes?

No, but we could track these things at run-time in debug mode. Would probably not be worth the extra code complexity added.

vittorioromeo

Some minor readability changes.

ChrisThrasher · 2023-04-04T22:09:15Z

Thanks for the feedback. I used structured bindings to get rid of all those double lookups.

By the way, here are some more places we can use structured bindings. Might submit a PR if others are eager to use structured bindings in more places.

ChrisThrasher added feature m:sfml-graphics labels Apr 2, 2023

ChrisThrasher added this to the 3.0 milestone Apr 2, 2023

ChrisThrasher requested review from Bromeon, eXpl0it3r and vittorioromeo April 2, 2023 23:09

ChrisThrasher force-pushed the text_construction branch 5 times, most recently from 502f0b2 to 1ad7841 Compare April 3, 2023 02:50

Bromeon linked an issue Apr 3, 2023 that may be closed by this pull request

Replace the default constructor of sf::Text with one that takes an sf::Font #2194

Closed

Bromeon reviewed Apr 3, 2023

View reviewed changes

Comment thread examples/joystick/Joystick.cpp Outdated

vittorioromeo requested changes Apr 4, 2023

View reviewed changes

Be explicit about when new keys are added to the map

ca70e6f

ChrisThrasher force-pushed the text_construction branch 3 times, most recently from 4301175 to d278b06 Compare April 4, 2023 22:08

Remove default sf::Text constructor

221eb2a

ChrisThrasher force-pushed the text_construction branch from d278b06 to 221eb2a Compare April 4, 2023 22:13

vittorioromeo approved these changes Apr 4, 2023

View reviewed changes

ChrisThrasher requested a review from Bromeon April 5, 2023 03:41

Bromeon approved these changes Apr 5, 2023

View reviewed changes

ChrisThrasher merged commit 2c99b33 into SFML:master Apr 5, 2023

ChrisThrasher deleted the text_construction branch April 5, 2023 15:55

This was referenced Apr 5, 2023

Remove default sf::Sprite constructor #2494

Merged

Remove default constructor for types that require a view into a resource #2507

Closed

ChrisThrasher referenced this pull request May 4, 2023

Remove default sf::Sprite constructor

29863b1

kimci86 mentioned this pull request Aug 5, 2023

Return reference instead of non-null pointer #2637

Merged

Uh oh!

Uh oh!

Conversation

ChrisThrasher commented Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

codecov Bot commented Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChrisThrasher commented Apr 3, 2023

Uh oh!

ChrisThrasher commented Apr 3, 2023

Uh oh!

Bromeon commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisThrasher commented Apr 3, 2023

Uh oh!

vittorioromeo commented Apr 4, 2023

Uh oh!

vittorioromeo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChrisThrasher commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ChrisThrasher commented Apr 2, 2023 •

edited

Loading

codecov Bot commented Apr 2, 2023 •

edited

Loading

Bromeon commented Apr 3, 2023 •

edited

Loading

ChrisThrasher commented Apr 4, 2023 •

edited

Loading