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.

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

[21184] Refactor XML Parser to return DynamicTypeBuilder instead of DynamicType #4970

Merged
merged 17 commits into from
Jul 3, 2024

Conversation

elianalf
Copy link
Contributor

@elianalf elianalf commented Jun 19, 2024

Description

This PR is a small refactor of the methods to get dynamic types from XML in order to return a DynamicTypeBuilder object instead of a DynamicType one. It also add a CLI flag to xtypes example to load types and profiles from an xml files.

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • Any new/modified methods have been properly documented using Doxygen.
  • N/A Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • ❌ Changes are backport compatible: they do NOT break ABI nor change library core behavior.
  • ❌ Changes are API compatible.
  • New feature has been added to the versions.md file (if applicable).
  • New feature has been documented/Current behavior is correctly described in the documentation.
    Related documentation PR: [21184] Adjust type in get_dynamic_type_builder_from_xml_by_name to return DynamicTypeBuilder Fast-DDS-docs#818 -->
  • N/A Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@elianalf elianalf added this to the v3.0.0 milestone Jun 19, 2024
@elianalf elianalf marked this pull request as ready for review June 19, 2024 10:06
@elianalf elianalf force-pushed the feature/refactor_get_dynamic_type_builder branch from b7b621c to 97ec783 Compare June 19, 2024 11:12
@elianalf elianalf force-pushed the feature/refactor_get_dynamic_type_builder branch 3 times, most recently from b42806b to 9b181e5 Compare June 20, 2024 14:31
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

Good refactor ! Leaving some suggestions.

In addition, couple of considerations:

  • Please, initialize type_discovered_(false) in the SubscriberApp.cpp since it was causing undefined behavior to me (I forgot to add this suggestion when the Xtypes example refactor was merged).
  • Also, I am getting a AddressSanitizer:DEADLYSIGNAL if I start the publisher with --xml-type (compiled with ASAN), we may need to investigate it further.

test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/unittest/dds/participant/ParticipantTests.cpp Outdated Show resolved Hide resolved
test/unittest/xmlparser/XMLProfileParserTests.cpp Outdated Show resolved Hide resolved
examples/cpp/xtypes/README.md Outdated Show resolved Hide resolved
examples/cpp/xtypes/PublisherApp.cpp Outdated Show resolved Hide resolved
@elianalf elianalf force-pushed the feature/refactor_get_dynamic_type_builder branch 2 times, most recently from f1064ba to 6cbc19c Compare June 24, 2024 12:59
@elianalf
Copy link
Contributor Author

elianalf commented Jun 25, 2024

* Also, I am getting a `AddressSanitizer:DEADLYSIGNAL` if I start the publisher with `--xml-type` (compiled with ASAN), we may need to investigate it further.

I have tried to reproduce the error, and it appears when the xml file is not uploaded through environment variable. Can you check if that is the case?
I will expand the README to clarify that using the --xml-types option requires setting an environment variable with the path to the XML file you wish to use.

@elianalf elianalf force-pushed the feature/refactor_get_dynamic_type_builder branch from 6cbc19c to 5602945 Compare June 25, 2024 13:29
@elianalf elianalf requested a review from Mario-DL June 25, 2024 13:30
@github-actions github-actions bot added ci-pending PR which CI is running labels Jun 25, 2024
@elianalf elianalf requested review from Mario-DL and removed request for Mario-DL July 2, 2024 08:49
@elianalf elianalf force-pushed the feature/refactor_get_dynamic_type_builder branch from c727706 to 418d2fe Compare July 2, 2024 12:28
@elianalf elianalf requested review from Mario-DL and removed request for Mario-DL July 2, 2024 12:29
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

Leaving unresolved just a couple of conversations above that I think were not included

@elianalf
Copy link
Contributor Author

elianalf commented Jul 2, 2024

Suggestions applied!

Leaving unresolved just a couple of conversations above that I think were not included

@elianalf elianalf requested a review from Mario-DL July 2, 2024 14:13
@elianalf elianalf force-pushed the feature/refactor_get_dynamic_type_builder branch from 82ac266 to 8294e3e Compare July 2, 2024 14:19
@elianalf elianalf requested review from Mario-DL and removed request for Mario-DL July 2, 2024 14:20
Mario-DL
Mario-DL previously approved these changes Jul 2, 2024
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

elianalf added 3 commits July 3, 2024 08:25
…etDynamicTypeByName to get a DynamicTypeBuilder

Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
…m xml

Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
elianalf added 14 commits July 3, 2024 08:25
Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
…le works

Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
…nullptr

Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
…TypeBuilder

Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com>
@elianalf elianalf force-pushed the feature/refactor_get_dynamic_type_builder branch from 8294e3e to b4b5edf Compare July 3, 2024 06:44
@elianalf elianalf requested a review from Mario-DL July 3, 2024 06:45
@elianalf elianalf added the ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. label Jul 3, 2024
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

LGTM

@EduPonz EduPonz removed the ci-pending PR which CI is running label Jul 3, 2024
@EduPonz EduPonz merged commit bc557b7 into master Jul 3, 2024
17 checks passed
@EduPonz EduPonz deleted the feature/refactor_get_dynamic_type_builder branch July 3, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants