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

Removed reference from input argument #76

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

jontje
Copy link
Contributor

@jontje jontje commented Feb 20, 2020

Fix for a bug that was accidently introduced in PR #58.

The bug caused nestled RAPID records to be wrongly parsed.

@jontje jontje requested a review from gavanderhoorn February 20, 2020 13:15
@gavanderhoorn
Copy link
Member

Could you provide some more insight into what went wrong and how the change introduced by #58 caused it?

Also: we need to add (unit) tests here. Good ones would catch such regressions.

@jontje
Copy link
Contributor Author

jontje commented Feb 21, 2020

Thanks for the review @gavanderhoorn 👍

Could you provide some more insight into what went wrong and how the change introduced by #58 caused it?

The method affected by this PR counts the number of times a character occur in a string, and it does that by:

  1. Use std::string::find_first_of.
  2. Removing the character.
  3. Repeat from 1. until reaching the end of the string.

Step 2. caused the method to alter the original string when a reference was used for the argument.

Example of the bug:

  • Simple RAPID record: [1,2,3] --> became 1,2,3 --> could still be parsed correctly.
  • Nestled RAPID record: [1,2,3,[4,5,6]] --> became 1,2,3,4,5,6 --> could not be parsed correctly.

Also: we need to add (unit) tests here. Good ones would catch such regressions.

Yes, that would be great. I have expanded my functional tests with a case to check this, but I only run them ad hoc.

@jontje jontje merged commit a5e9244 into ros-industrial:master Feb 21, 2020
@jontje jontje deleted the fix_nested_record branch February 21, 2020 07:31
@gavanderhoorn
Copy link
Member

Tracking tests in #77.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants