Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make transport public headers private [10002] #1577

Merged
merged 30 commits into from
Feb 26, 2021
Merged

Make transport public headers private [10002] #1577

merged 30 commits into from
Feb 26, 2021

Conversation

JLBuenoLopez
Copy link
Contributor

No description provided.

@JLBuenoLopez JLBuenoLopez added the skip-ci Automatically pass CI label Nov 23, 2020
@JLBuenoLopez JLBuenoLopez changed the title Make transport public headers private Make transport public headers private [10002] Nov 23, 2020
MiguelCompany
MiguelCompany requested changes Nov 23, 2020
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

I think we should take the chance to clean-up all the headers under include/fastdds/rtps/transport/

Start by doing an uncrustify over all of them

@JLBuenoLopez
Copy link
Contributor Author

JLBuenoLopez commented Nov 23, 2020 •
edited
Loading

Start by doing an uncrustify over all of them

@MiguelCompany,

Done in dabe29f

MiguelCompany
MiguelCompany requested changes Nov 23, 2020
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

Partial review

MiguelCompany
MiguelCompany requested changes Nov 23, 2020
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

Partial review 2: all public headers reviewed

MiguelCompany
MiguelCompany requested changes Nov 24, 2020
View reviewed changes
@rsanchez15 rsanchez15 force-pushed the feature/increase-coverage branch from 2fb6834 to 80b27f1 Compare November 27, 2020 08:19
@JLBuenoLopez JLBuenoLopez force-pushed the fix/remove_public_headers_transport_layer branch 2 times, most recently from 8175c55 to 4d7eeff Compare December 4, 2020 11:19
MiguelCompany
MiguelCompany requested changes Dec 4, 2020
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

Copy assignment on public headers need RTPS_DllAPI

Base automatically changed from feature/increase-coverage to master December 9, 2020 11:11
@imontesino imontesino force-pushed the fix/remove_public_headers_transport_layer branch 2 times, most recently from 70e5157 to 80b7478 Compare December 23, 2020 12:40
@imontesino imontesino removed the skip-ci Automatically pass CI label Dec 23, 2020
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Linux aarch64Build Status
  • Mac Build Status
  • Windows Build Status

MiguelCompany
MiguelCompany requested changes Jan 12, 2021
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

Apart from my comments below, a blackbox test checking copy constructors and assignment operators of the transport descriptors would be nice

@JLBuenoLopez JLBuenoLopez marked this pull request as draft January 13, 2021 08:20
@JLBuenoLopez JLBuenoLopez added the skip-ci Automatically pass CI label Jan 13, 2021
@JLBuenoLopez
Copy link
Contributor Author

Converted to draft and labeled skip-ci until introducing the Blackbox tests asked by the reviewer.

@jparisu jparisu removed the skip-ci Automatically pass CI label Jan 20, 2021
@jparisu jparisu marked this pull request as ready for review January 20, 2021 11:48
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Linux aarch64Build Status
  • Mac Build Status
  • Windows Build Status

@jparisu jparisu added the no-test Skip CI tests if PR marked with this label label Feb 2, 2021
MiguelCompany
MiguelCompany requested changes Feb 2, 2021
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

  1. I would name the macro FASTDDS_TODO_BEFORE
  2. Fix the reported linter issues

MiguelCompany
MiguelCompany previously approved these changes Feb 3, 2021
View reviewed changes
@eProsima eProsima deleted a comment from richiware Feb 3, 2021
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Linux aarch64Build Status
  • Mac Build Status
  • Windows Build Status

Ignacio Montesino and others added 14 commits February 4, 2021 12:52
@MiguelCompany
rerun CI
e8f6ed5 
Signed-off-by: Ignacio Montesino <ignaciomontesino@eprosima.com>
@JLBuenoLopez @MiguelCompany
Refs #9957: applied review suggestions
3fb1406 
Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com>
@JLBuenoLopez @MiguelCompany
Refs #9957: SHM Blackbox test
0643d4b 
Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com>
@JLBuenoLopez @MiguelCompany
Refs #9957: TCPv4 Blackbox test (preliminar)
6728e33 
Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com>
@JLBuenoLopez @MiguelCompany
Refs #9957: add comparison operator to TransportDescriptors
4ffc205 
Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com>
@JLBuenoLopez @MiguelCompany
Refs #9957: use comparison operator in SHM test
eb2752d 
Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com>
@MiguelCompany
Refs #9957: fix == operator in TCPv4Transport
0ec7cba 
Signed-off-by: jparisu <javierparis@eprosima.com>
@MiguelCompany
Refs #9957: add test for copy and == operator for TransportDescriptor…
6320051 
… classes

Signed-off-by: jparisu <javierparis@eprosima.com>
@MiguelCompany
Refs #9957: uncrustify
3eb625a 
Signed-off-by: jparisu <javierparis@eprosima.com>
@MiguelCompany
Refs #9957: uncrustify bis
756f50d 
Signed-off-by: jparisu <javierparis@eprosima.com>
@MiguelCompany
Refs #9957: new TODO macro and fix typo
8ffcb7a 
Signed-off-by: jparisu <javierparis@eprosima.com>
@MiguelCompany
Refs #9957: apply changes
9dd3e6d 
Signed-off-by: jparisu <javierparis@eprosima.com>
@MiguelCompany
Refs #9957: uncrustify
799d7a2 
Signed-off-by: jparisu <javierparis@eprosima.com>
@MiguelCompany
Refs 10002. Fixed DDSBlackboxTestsDataSharing.
0ddc9c7 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany MiguelCompany force-pushed the fix/remove_public_headers_transport_layer branch from b20472a to 0ddc9c7 Compare February 4, 2021 12:07
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Linux aarch64Build Status
  • Mac Build Status
  • Windows Build Status

MiguelCompany
MiguelCompany previously approved these changes Feb 9, 2021
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

LGTM

@MiguelCompany MiguelCompany added the ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. label Feb 9, 2021
@MiguelCompany MiguelCompany added this to the v2.3.0 milestone Feb 9, 2021
@MiguelCompany MiguelCompany mentioned this pull request Feb 19, 2021
Merged
@EduPonz EduPonz removed the no-test Skip CI tests if PR marked with this label label Feb 25, 2021
@EduPonz
Copy link

EduPonz commented Feb 25, 2021

@richiprosima Please test this

@MiguelCompany
Copy link
Member

@richiprosima Please test mac

@MiguelCompany
Copy link
Member

@richiprosima Please test this

@MiguelCompany
Refs 10002. Fix BalckboxTestsTransportUDP.
fc7eab7 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany MiguelCompany merged commit bdbd72c into master Feb 26, 2021
@MiguelCompany MiguelCompany deleted the fix/remove_public_headers_transport_layer branch February 26, 2021 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jparisu jparisu jparisu left review comments

@MiguelCompany MiguelCompany MiguelCompany approved these changes

Assignees
No one assigned
Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Milestone
v2.3.0
Development

Successfully merging this pull request may close these issues.
6 participants
@JLBuenoLopez @richiware @EduPonz @MiguelCompany @jparisu @imontesino