-
Notifications
You must be signed in to change notification settings - Fork 683
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
Changed IPNetwork
copy assignment implementation to copy-and-swap.
#1681
base: dev
Are you sure you want to change the base?
Conversation
…he internal network. This change fixes use of memory after being freed in case of self-assignment and provides a strong exception guarantee.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1681 +/- ##
==========================================
- Coverage 83.16% 83.15% -0.01%
==========================================
Files 277 277
Lines 48201 48192 -9
Branches 9932 9941 +9
==========================================
- Hits 40086 40075 -11
+ Misses 7266 7242 -24
- Partials 849 875 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Common++/header/IpAddress.h
Outdated
m_IPv4Network = std::unique_ptr<IPv4Network>(new IPv4Network(other)); | ||
|
||
auto newNetwork = std::unique_ptr<IPv4Network>(new IPv4Network(other)); | ||
m_IPv4Network = std::move(newNetwork); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary to use std::move
here since a smart pointer is already cheap.
I would just use m_IPv4Network = std::unique_ptr<IPv4Network>(new IPv4Network(other));
for clearance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the operations can be merged. Both should compile down to the same tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dimi1010 I also think it will be the same after the compilation optimization. Then could you just merge the lines, so it looks cleaner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be good now.
This change fixes use of memory after being freed in case of self-assignment and provides a strong exception guarantee.