close
Skip to content

Enable move semantics for socket types#2658

Merged
ChrisThrasher merged 2 commits into
masterfrom
network_move_semantics
Aug 27, 2023
Merged

Enable move semantics for socket types#2658
ChrisThrasher merged 2 commits into
masterfrom
network_move_semantics

Conversation

@ChrisThrasher

@ChrisThrasher ChrisThrasher commented Aug 22, 2023

Copy link
Copy Markdown
Member

Description

This one wasn't entirely trivial because I had to write custom move operations for sf::Socket to ensure that a socket isn't closed after it's moved-from. The move operations ensure that the moved-from socket is given an invalid socket handle so that when close is called nothing happens. Enabling move semantics for sf::Socket had the downstream affect of enabling move semantics for 3 different derived classes. It's great how the type trait tests so easily detect and confirm that fact.

As a prerequisite I wrote up some sf::Socket tests since this class was lacking any.

@codecov

codecov Bot commented Aug 22, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2658 (95b0e5e) into master (bbb6f60) will increase coverage by 0.06%.
The diff coverage is 93.33%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2658      +/-   ##
==========================================
+ Coverage   30.68%   30.74%   +0.06%     
==========================================
  Files         229      229              
  Lines       19718    19733      +15     
  Branches     4725     4728       +3     
==========================================
+ Hits         6050     6067      +17     
- Misses      12915    13045     +130     
+ Partials      753      621     -132     
Files Changed Coverage Δ
include/SFML/Network/Socket.hpp 50.00% <ø> (ø)
src/SFML/Network/Socket.cpp 85.33% <93.33%> (+7.00%) ⬆️

... and 53 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 bbb6f60...95b0e5e. Read the comment docs.

@ChrisThrasher ChrisThrasher requested a review from Bromeon August 22, 2023 15:26

@Rosme Rosme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That looks good to me.

@kimci86 kimci86 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nitpicking. Otherwise it looks correct.

Comment thread test/Network/Socket.test.cpp Outdated
Comment thread test/Network/Socket.test.cpp Outdated
Comment thread src/SFML/Network/Socket.cpp
@ChrisThrasher ChrisThrasher force-pushed the network_move_semantics branch from 43b07d3 to b8044f1 Compare August 23, 2023 20:01
@ChrisThrasher

Copy link
Copy Markdown
Member Author

I included the Clang CI fix in this PR because other CI is guaranteed to fail. See #2661 for more information.

@ChrisThrasher ChrisThrasher requested a review from kimci86 August 23, 2023 20:04
@kimci86

kimci86 commented Aug 23, 2023

Copy link
Copy Markdown
Contributor

I included the Clang CI fix in this PR because

You can edit this PR base branch to clang_ci_workaround to make the changes more specific. It will become master again automatically once clang_ci_workaround gets merged.

@ChrisThrasher ChrisThrasher force-pushed the network_move_semantics branch from b8044f1 to 0d1f7f0 Compare August 23, 2023 20:37
@ChrisThrasher ChrisThrasher force-pushed the network_move_semantics branch from 0d1f7f0 to b168c55 Compare August 25, 2023 17:40
@ChrisThrasher

Copy link
Copy Markdown
Member Author

I removed the CI failure workarounds from this PR since it appears that GitHub has fixed the issues.

@ChrisThrasher ChrisThrasher force-pushed the network_move_semantics branch from b168c55 to 95b0e5e Compare August 27, 2023 03:10
@ChrisThrasher ChrisThrasher merged commit e5c41c4 into master Aug 27, 2023
@ChrisThrasher ChrisThrasher deleted the network_move_semantics branch August 27, 2023 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants