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

[rclcpp_lifecycle] Change uint8_t iterator variables to size_t #1461

Merged
merged 2 commits into from
Nov 17, 2020
Merged

[rclcpp_lifecycle] Change uint8_t iterator variables to size_t #1461

merged 2 commits into from
Nov 17, 2020

Conversation

brawner
Copy link
Contributor

@brawner brawner commented Nov 16, 2020

This is a small theoretical issue I found. The type of states_size and transitions_size is unsigned int, but the for loop int type is uint8_t. While default_state_machine has only 11 states and 25 transitions, looping with a small integer type that is compared to a larger type could lead to an infinite loop. While size_t here is large enough to avoid problems, I'd also be happy to use also unsigned int.

Signed-off-by: Stephen Brawner brawner@gmail.com

@brawner
Change uint8_t iterator variables to size_t
1a28c05 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Copy link
Contributor Author

brawner commented Nov 16, 2020

Testing --packages-select rclcpp_lifecycle

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

I'd also be happy to use also unsigned int.

Given that states_size is defined as an unsigned int, I suggest we stick with that type here. It just ensures that we are comparing the same type always. I don't think this change has any practical difference on the platforms we support, but it makes it easy to tell that we are doing the correct thing.

@brawner
Change to unsigned int
5320559 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Copy link
Contributor Author

brawner commented Nov 17, 2020

Sounds good, fixed.

@brawner
Copy link
Contributor Author

brawner commented Nov 17, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@brawner brawner merged commit add6d61 into master Nov 17, 2020
@delete-merged-branch delete-merged-branch bot deleted the brawner/for-loop-uint8_t-unsigned-int branch November 17, 2020 19:22
@brawner brawner added this to Proposed in Foxy Patch Release 4 via automation Nov 17, 2020
@brawner brawner moved this from Proposed to Needs backport in Foxy Patch Release 4 Nov 17, 2020
@brawner brawner mentioned this pull request Nov 18, 2020
Merged
jacobperron pushed a commit that referenced this pull request Dec 8, 2020
@brawner @jacobperron
[rclcpp_lifecycle] Change uint8_t iterator variables to size_t (#1461)
2a41899 
* Change uint8_t iterator variables to size_t

Signed-off-by: Stephen Brawner <brawner@gmail.com>

* Change to unsigned int

Signed-off-by: Stephen Brawner <brawner@gmail.com>
@jacobperron jacobperron mentioned this pull request Dec 8, 2020
Merged
jacobperron added a commit that referenced this pull request Dec 8, 2020
@jacobperron @brawner
[rclcpp_lifecycle] Change uint8_t iterator variables to size_t (#1461) (
b132a2b 
#1488)

* Change uint8_t iterator variables to size_t

Signed-off-by: Stephen Brawner <brawner@gmail.com>

* Change to unsigned int

Signed-off-by: Stephen Brawner <brawner@gmail.com>

Co-authored-by: brawner <brawner@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@brawner @clalancette