Skip to content

SocketAddress rework #12683

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

Merged
merged 2 commits into from
Mar 27, 2020
Merged

SocketAddress rework #12683

merged 2 commits into from
Mar 27, 2020

Conversation

kivaisan
Copy link
Contributor

Summary of changes

Original work: #12468 . This PR contains also the UT fix.

  • Add optimised constexpr default constructor. Default construction was previously by a heavyweight defaulted nsapi_addr_t parameter.
  • Remove deprecated resolving constructor.
  • Take nsapi_addr_t inputs by constant reference rather than value.
  • Inline the trivial getters and setters.
  • Use unique_ptr to manage the text buffer.
  • Make operator bool explicit.
  • Optimise some methods.
  • Update to C++11 style (default initialisers, nullptr etc)

Impact of changes

  • Constructor deprecated in Mbed OS 5.1 removed.
  • Code size reductions, particularly on default initialisation.
  • Implicit assignments to bool or int or others no longer possible - any existing code which does not compile is most likely an error. (if (sockaddr) is still fine - such "contextual conversions to bool" can use the explicit operator).

Migration actions required

  • Code attempting resolution by passing a hostname to SocketAddress's constructor must be modified to use NetworkInterface::gethostbyname or NetworkStack::gethostbyname.
  • Code failing due to the now-explicit bool operator should be reviewed to check intent.

Documentation

n/a


Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[X] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


kjbracey and others added 2 commits March 24, 2020 11:05
* Add optimised constexpr default constructor. Default construction
  was previously by a heavyweight defaulted `nsapi_addr_t` parameter.
* Remove deprecated resolving constructor.
* Take `nsapi_addr_t` inputs by constant reference rather than value.
* Inline the trivial getters and setters.
* Use `unique_ptr` to manage the text buffer.
* Make `operator bool` explicit.
* Optimise some methods.
* Update to C++11 style (default initialisers, nullptr etc)
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 24, 2020

As previous PR was approved, although I made a comment there about having one commit for this rework is not ideal for future maintenance. @kivaisan Would you be able to split the first commit ?

I started CI meanwhile

@ciarmcom ciarmcom requested review from a team March 24, 2020 10:00
@ciarmcom
Copy link
Member

@kivaisan, thank you for your changes.
@ARMmbed/mbed-os-tools @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@mbed-ci
Copy link

mbed-ci commented Mar 24, 2020

Test run: FAILED

Summary: 1 of 6 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 24, 2020

Test failed, but I saw this recently in another PR.

DISCO_L475VG_IOT01A-ARMC6.features-storage-tests-kvstore-direct_access_devicekey_test fails with hard fault. This looks related to minimal printf however it fails there as well (opened PR with a fix)

@ARMmbed/mbed-os-storage Please review, looks like this is on master as well.

@SeppoTakalo
Copy link
Contributor

Same failure as in the minimal-printf PR.
However, as both PRs don't really touch any areas that can affect the test, I suspect HW failure.
Can someone remove that particular board from CI.
Maybe erase-program cycle is now over.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 24, 2020

Just read your comment there, I notified the test team to check the board.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 25, 2020

test restarted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants