-
Notifications
You must be signed in to change notification settings - Fork 3k
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
C++11-ify virtualisation in netsocket #12487
Conversation
@kjbracey-arm, thank you for your changes. |
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.
LGTM
CI started |
Test run: FAILEDSummary: 1 of 4 test jobs failed Failed test jobs:
|
4ee4a18
to
53539d4
Compare
Fixed unit tests |
Pull request has been modified.
CI started |
Test run: FAILEDSummary: 2 of 11 test jobs failed Failed test jobs:
|
Spurious test system failures? |
yes, looks like that. Restarted both |
Why is this part of the commit as well? It's not that easy to find what we have missed. Shouldn't this be separate commit or even PR? The rest looks fine to me. |
Previous change that removed string-based APIs missed `NetworkStackWrapper::get_ip_address`. Remove string-based method (which is not overriding anything in `NetworkStack`) and add missing binary form to implement `NetworkStack::get_ip_address`.
Use `override` and `final` where appropriate, and eliminate unnecessary `virtual`. Some other C++11 simplifications. Reduces code size.
53539d4
to
d8d35ed
Compare
I've split the commits. No end result change. |
CI started |
Test run: FAILEDSummary: 2 of 7 test jobs failed Failed test jobs:
|
Most likely both failures are known. I have a fix for tests (thread timeout), and client - not yet how to proceed there |
Ci restarted (tests should be fixed) |
Test run: SUCCESSSummary: 7 of 7 test jobs passed |
Summary of changes
Use
override
andfinal
where appropriate, and eliminate unnecessaryvirtual
.Fixes some mismatches in string-based API removal (#11942)
Some other C++11 simplifications.
Reduces code size.
Marking as breaking change because it fixes up some work in a previous breaking change.
Impact of changes
No new implications.
Migration actions required
No new actions.
Documentation
None.
Pull request type
Test results
Reviewers
@michalpasztamobica