Skip to content

SocketAddress rework #12468

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

Closed
wants to merge 1 commit into from
Closed

SocketAddress rework #12468

wants to merge 1 commit into from

Conversation

kjbracey
Copy link
Contributor

Summary of changes

  • 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
Copy link
Contributor Author

This relates to #12455 - that compiler fault was probably triggered by the complexity of SocketAddress's default construction. (Construct a defaulted temporary nsapi_addr_t, then pass it by value to a constructor).

@mergify mergify bot added the needs: work label Feb 19, 2020
@ciarmcom ciarmcom requested review from a team February 19, 2020 12:00
@ciarmcom
Copy link
Member

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

* 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)
@mergify mergify bot added needs: CI and removed needs: work labels Mar 24, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 24, 2020

Unittest failed (will be reported soon)

Lot of fine fixes here but squashing them into one, makes it hard to fix issues in the future if anything found in this commit.. I would split this one into at least 3 as stated in the Impact of changes (3 points) - they do not look related besides "rework"

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 24, 2020

@ARMmbed/mbed-os-ipcore as @kjbracey-arm is OoO, is there anyone else who could push this forward ?

@kivaisan
Copy link
Contributor

I'll check the UT.

@mbed-ci
Copy link

mbed-ci commented Mar 24, 2020

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@kivaisan kivaisan mentioned this pull request Mar 24, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 24, 2020

Closing to continue in the referenced ^^ PR

@0xc0170 0xc0170 closed this Mar 24, 2020
@mergify mergify bot removed the needs: CI label Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants