Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

@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
# for free to join this conversation on GitHub. Already have an account? # 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