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

[15417] Group set_qos_from_attributes free functions into a separate file #2914

Merged
merged 25 commits into from
Sep 2, 2022
Merged

[15417] Group set_qos_from_attributes free functions into a separate file #2914

merged 25 commits into from
Sep 2, 2022

Conversation

MRicoIE2CS
Copy link
Contributor

Description

set_qos_from_attributes() are defined as static free functions. And there is the need to access them from outside the .cpp where they are defined.
This PR should satisfy the following:

  • Provide a header and a source
  • Put the new files in src/cpp/fastdds/utils
  • Add the free functions into the eprosima::fastdds::dds::utils namespace.
  • The new functions cannot be static. We won't inline them either.
  • Refactor usage of the functions.

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • [N/A] Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added.
  • Any new/modified methods have been properly documented using Doxygen.
  • Fast DDS test suite has been run locally.
  • Changes are ABI compatible.
  • Changes are API compatible.
  • [N/A] Documentation builds and tests pass locally.
  • [N/A] New feature has been added to the versions.md file (if applicable).
  • [N/A] New feature has been documented/Current behavior is correctly described in the documentation.

Reviewer Checklist

  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@MRicoIE2CS
Refs #15417: New QosConverters.hpp and .cpp in src/cpp/fastdds/utils …
3d09b09 
…folder.

Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs 15417: set_qos_from_attributes imported from PublisherImpl.
b6ce99c 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs 15417: set_qos_from_attributes commented in PublisherImpl, and #…
0a24374 
…included. src/cpp/CMakeLists.txt modified to compile new source.

Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: Some CMakeLists.txt from test/ modified to compile new s…
2442041 
…ource.

Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: Commented function erased, and not-used using and #inclu…
fd923a6 
…de erased.

Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: set_qos_from_attributes imported from DomainParticipantF…
56b21aa 
…actory.

Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: set_qos_from_attributes function in DomainParticipantFac…
fcc78f4 
…tory erased, and not-used #include erased..

Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: set_qos_from_attributes and set_attributes_from_qos impo…
a171fa5 
…rted from DomainParticipantImpl.

Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: set_qos_from_attributes and set_attributes_from_qos func…
bcf59a4 
…tions in DomainParticipantImpl erased.

Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: set_qos_from_attributes imported from SubscriberImpl.
73cf75c 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: set_qos_from_attributes function in SubscriberImpl erased.
36e356e 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: Moved the new functions of QosConverters to namespace ::…
0a8a85c 
…utils::

Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: Calling set_qos_from_attributes and set_attributes_from_…
dd364b0 
…qos pointing to ::utils:: namespace.

Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: Documentation of the free functions.
6f84ddc 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Copy link
Contributor Author

@richiprosima please test this

@MRicoIE2CS
Copy link
Contributor Author

@richiprosima please test this again

@JLBuenoLopez
Copy link
Contributor

@richiprosima please test mac

1 similar comment
@JLBuenoLopez
Copy link
Contributor

@richiprosima please test mac

JLBuenoLopez
JLBuenoLopez suggested changes Aug 24, 2022
View reviewed changes
src/cpp/fastdds/utils/QosConverters.cpp Outdated Show resolved Hide resolved
src/cpp/fastdds/utils/QosConverters.cpp Outdated Show resolved Hide resolved
src/cpp/fastdds/utils/QosConverters.hpp Outdated Show resolved Hide resolved
src/cpp/fastdds/utils/QosConverters.hpp Outdated Show resolved Hide resolved
src/cpp/fastdds/utils/QosConverters.hpp Outdated Show resolved Hide resolved
src/cpp/fastdds/utils/QosConverters.hpp Outdated Show resolved Hide resolved
src/cpp/fastdds/utils/QosConverters.hpp Outdated Show resolved Hide resolved
src/cpp/fastdds/utils/QosConverters.hpp Outdated Show resolved Hide resolved
src/cpp/fastdds/utils/QosConverters.hpp Show resolved Hide resolved
src/cpp/fastdds/utils/QosConverters.cpp Show resolved Hide resolved
@MRicoIE2CS MRicoIE2CS mentioned this pull request Aug 25, 2022
Merged
11 tasks
@MRicoIE2CS
Refs #15417: Year change.
4080389 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: Move description to header file.
344dd82 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: Blanc lines and guard condition.
78ce441 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: Blanc lines and reference to each specific QoS and Attri…
0a5c8bf 
…butes used on each method.

Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: #includes.
c4a280c 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: using std::string.
82ac123 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: uncrustify.
33e19eb 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
JLBuenoLopez
JLBuenoLopez reviewed Aug 25, 2022
View reviewed changes
Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

A couple of nitpicks to improve the doxydoc and comply with the code style.

src/cpp/fastdds/utils/QosConverters.cpp Outdated Show resolved Hide resolved
src/cpp/fastdds/utils/QosConverters.hpp Outdated Show resolved Hide resolved
@MRicoIE2CS
Refs #15417: #includes order.
560d16c 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: Doxygen comments.
d8fa2d1 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS
Refs #15417: Doxygen comments.
9edf728 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
@MRicoIE2CS MRicoIE2CS force-pushed the refactor/group_set_qos_from_attributes branch from 77c1c7a to 9edf728 Compare August 25, 2022 10:26
@MRicoIE2CS
Refs #15417: Got your blank line.
b1befe3 
Signed-off-by: Mikel Rico <mikelrico@eprosima.com>
JLBuenoLopez
JLBuenoLopez approved these changes Aug 25, 2022
View reviewed changes
Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, @MRicoIE2C!

I will launch another CI run previous to be merged, though I am not expecting any significant difference from the previous runs.

@JLBuenoLopez
Copy link
Contributor

@richiprosima please test this

@JLBuenoLopez JLBuenoLopez added the ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. label Aug 25, 2022
@JLBuenoLopez JLBuenoLopez added this to the v2.8.0 milestone Aug 25, 2022
@MiguelCompany
Copy link
Member

@richiprosima Please test mac

@MiguelCompany MiguelCompany merged commit 7ccc7a7 into master Sep 2, 2022
@MiguelCompany MiguelCompany deleted the refactor/group_set_qos_from_attributes branch September 2, 2022 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JLBuenoLopez JLBuenoLopez JLBuenoLopez approved these changes

@EduPonz EduPonz Awaiting requested review from EduPonz

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.8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@MRicoIE2CS @JLBuenoLopez @MiguelCompany