Skip to content

operator+= and operator-= for Duration#1988

Merged
clalancette merged 3 commits intoros2:rollingfrom
tylerjw:duration_operators
Sep 2, 2022
Merged

operator+= and operator-= for Duration#1988
clalancette merged 3 commits intoros2:rollingfrom
tylerjw:duration_operators

Conversation

@tylerjw
Copy link
Copy Markdown
Contributor

@tylerjw tylerjw commented Aug 11, 2022

Signed-off-by: Tyler Weaver maybe@tylerjw.dev

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

adding operators makes sense.
but this PR cannot even be built, can you address build failure and add some test?

@alsora
Copy link
Copy Markdown
Collaborator

alsora commented Aug 11, 2022

LGTM with the changes requested by Tomoya

@tylerjw
Copy link
Copy Markdown
Contributor Author

tylerjw commented Aug 11, 2022

Thank you for the review, I'll add the tests and accept the tests tonight when I'm back at my computer. I'm sorry I didn't test this before submitting this.

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
@tylerjw
Copy link
Copy Markdown
Contributor Author

tylerjw commented Aug 12, 2022

I fixed the compile errors, added other operators that I noticed were missing, and added tests for it all.

@tylerjw tylerjw requested a review from fujitatomoya August 12, 2022 03:33
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@tylerjw thank you for your contribution. i got a few comments, could you share your thoughts? I was thinking that basically operator -/+ extension would be enough.

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
@tylerjw
Copy link
Copy Markdown
Contributor Author

tylerjw commented Aug 12, 2022

@fujitatomoya to make this PR easier I reduced the scope to only +=, -=, *=. I will open a second PR for /, /=. Thinking about the units though you are correct that it makes no sense to have seconds * seconds unless we had a seconds ^ 2 type we could assign into.

@tylerjw
Copy link
Copy Markdown
Contributor Author

tylerjw commented Aug 12, 2022

As we have operators that work on double type scale values, would it make sense to also have integer type scale values operators? If we did, would uint64_t be the right type to use?

With the current implementation if someone writes:

auto x = rclcpp::Duration::from_seconds(0.1) * 33554435;

The cast of of the integer value 33554435 to double would result in 33554432 +/- 4. While there is a long double type that would make this situation better (would require larger numbers to result in loss of precision) if we offered operators that took a large integer scale value the user would receive more precision. I don't have any examples in my code of huge integer scale values being multiplied by a duration so maybe this is too much of an edge case to care about?

@fujitatomoya
Copy link
Copy Markdown
Collaborator

The cast of of the integer value 33554435 to double would result in 33554432 +/- 4. While there is a long double type that would make this situation better (would require larger numbers to result in loss of precision) if we offered operators that took a large integer scale value the user would receive more precision. I don't have any examples in my code of huge integer scale values being multiplied by a duration so maybe this is too much of an edge case to care about?

thanks for the information.
technically this is correct. depends on decimal digits of precision, it loses some scale when it implicitly cast the value to double.
beside, internally it casts the scale into long double to keep the precision.

long double scale_ld = static_cast<long double>(scale);
return Duration::from_nanoseconds(
static_cast<rcl_duration_value_t>(
static_cast<long double>(rcl_duration_.nanoseconds) * scale_ld));

but as you suggested, probably it is too much... i would not have the case either.
IMO, we would want to keep the interface as it is, instead of having the high precision for uncommon case?.

@clalancette
Copy link
Copy Markdown
Contributor

CI:

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants