operator+= and operator-= for Duration

[tylerjw]

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

[fujitatomoya]

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

[alsora]

LGTM with the changes requested by Tomoya

[tylerjw]

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.

[tylerjw]

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

[fujitatomoya]

@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.

[tylerjw]

@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]

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]

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.

rclcpp/rclcpp/src/rclcpp/duration.cpp

Lines 219 to 222 in b953bdd

	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]

CI:

• Linux
• Linux-aarch64
• Windows