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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove defaulted or empty, non-virtual destructors #2312 #2321

Closed
wants to merge 3 commits into from

Conversation

benjamintli
Copy link
Contributor

@benjamintli benjamintli commented Dec 27, 2022

Thanks a lot for making a contribution to SFML! 馃檪

Before you create the pull request, we ask you to check the follow boxes. (For small changes not everything needs to ticked, but the more the better!)

  • Has this change been discussed on the forum or in an issue before?
  • Does the code follow the SFML Code Style Guide?
  • Have you provided some example/test code for your changes?
  • If you have additional steps which need to be performed list them as tasks!

Description

Please describe your pull request.

resolves #2312

Tasks

  • Tested on Linux
  • Tested on Windows
  • Tested on macOS
  • Tested on iOS
  • Tested on Android

@codecov
Copy link

codecov bot commented Dec 27, 2022

Codecov Report

Merging #2321 (dc8bec1) into master (7884efc) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #2321    +/-   ##
========================================
  Coverage   14.64%   14.64%            
========================================
  Files         214      216     +2     
  Lines       15106    15106            
  Branches     4029     4029            
========================================
  Hits         2212     2212            
+ Misses      12761    12550   -211     
- Partials      133      344   +211     
Impacted Files Coverage 螖
include/SFML/Network/SocketSelector.hpp 0.00% <酶> (酶)
include/SFML/Window/Cursor.hpp 0.00% <酶> (酶)
src/SFML/Graphics/Shader.cpp 0.19% <酶> (酶)
src/SFML/Network/SocketSelector.cpp 0.00% <酶> (酶)
src/SFML/Window/Cursor.cpp 0.00% <酶> (酶)
src/SFML/Audio/Music.cpp 0.00% <0.00%> (酶)
src/SFML/Audio/Sound.cpp 0.00% <0.00%> (酶)
src/SFML/Network/Ftp.cpp 0.00% <0.00%> (酶)
src/SFML/Network/Http.cpp 74.50% <0.00%> (酶)
src/SFML/Audio/ALCheck.cpp 0.00% <0.00%> (酶)
... and 56 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 7884efc...dc8bec1. Read the comment docs.

@eXpl0it3r eXpl0it3r added this to Backlog in SFML 3.0.0 via automation Dec 27, 2022
@eXpl0it3r eXpl0it3r added this to the 3.0 milestone Dec 27, 2022
@eXpl0it3r eXpl0it3r moved this from Backlog to Clean Up Changes in SFML 3.0.0 Dec 27, 2022
ChrisThrasher
ChrisThrasher previously approved these changes Dec 28, 2022
@ChrisThrasher ChrisThrasher dismissed their stale review December 28, 2022 23:38

Premature approval

@ChrisThrasher
Copy link
Member

#2312 (comment)

@kimci86 made a great point in the issue thread how this type needs an explicitly declared destructor given how it's being implemented. I was able to corroborate this by simply attempting to construct an instead of sf::Cursor and see the build fail. I think coincidentally this passed CI because an sf::Cursor is never constructed (and thus destroyed) anywhere in the repo itself so we were able to omit a destructor without that issue being detected.

@ChrisThrasher
Copy link
Member

I'm closing this PR since we can't easily remove these destructors after all.

@eXpl0it3r eXpl0it3r moved this from Clean Up Changes to Done in SFML 3.0.0 Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
SFML 3.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

Remove defaulted or empty, non-virtual destructors
4 participants