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

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

Closed
3 tasks done
ChrisThrasher opened this issue Apr 9, 2023 · 2 comments
Closed
3 tasks done
Labels
Milestone

Comments

@ChrisThrasher
Copy link
Member

ChrisThrasher commented Apr 9, 2023

Subject of the issue

#2486 and #2494 remove default constructors for sf::Text and sf::Sprite which are types that are not usable unless they contain a pointer to a valid resource. Those resources are an sf::Font and a sf::Texture respectively. The rationale is that by allowing for default construction, we're making it too easy for users to write bugs where they forget to assign the correct resource to a given type. By removing the default constructor, we force users to provide that resource upon construction so we can be sure that instances of these types are valid at all points in time. The convenience of a default constructor is not worth the potentially incorrect code someone may write. It's not possible to set these pointers to nullptr after construction so why let them be null upon construction?

Here's a list of types that may need to get this same treatment before SFML 3.0.0.

I believe there are no more than just these three but I could have missed one. sf::Sound is harder because its copy assignment operator will set the underlying sf::SoundBuffer to nullptr so it actually have its resource set to nullptr after construction unlike the other two. A solution for sf::Sound may require altering how copy semantics work.

@ChrisThrasher ChrisThrasher added this to the 3.0 milestone Apr 9, 2023
@ChrisThrasher
Copy link
Member Author

ChrisThrasher commented Apr 9, 2023

@kimci86 pointed out how the copy semantics of sf::Sound don't actually leave the instance in a state with a null SoundBuffer. However I did discover sf::Sound::resetBuffer which sets the underlying sound buffer to null. If the API of sf::Sound explicitly allows for nullifying its underlying resource then it's valid for sf::Sound::Sound() to default to a null sound buffer.

    ////////////////////////////////////////////////////////////
    /// \brief Reset the internal buffer of the sound
    ///
    /// This function is for internal use only, you don't have
    /// to use it. It is called by the sf::SoundBuffer that
    /// this sound uses, when it is destroyed in order to prevent
    /// the sound from using a dead buffer.
    ///
    ////////////////////////////////////////////////////////////
    void resetBuffer();

It appears that sf::Sound and sf::SoundBuffer containers pointers to each other to address potential lifetime issues where the sound buffer is destroyed before the sound. Is this a behavior we want to keep in spite of it differing from the ownership model of sf::Text and sf::Sprite? Do we want sf::Text and sf::Sprite to adopt this ownership model instead?

@ChrisThrasher
Copy link
Member Author

362f37473

This machinery was added in 2010 and very little has changed about sf::SoundBuffer since then so I think it's reasonable to revisit this decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

No branches or pull requests

1 participant