Add parenthesis around the argument in time conversion macros defined in time.h

[meyerj]

... to protect against unexpected results of calls with sum arguments, or with other operators that have a lower precedence than the multiplication or division operators.

Related to ros2/rclcpp#1188:

There is another problem with the definition of time-related macros defined in rcutils/time.h which are also indirectly used for the Duration and Time implementations in rclcpp: They don't have parentheses around the macro argument:

/// Convenience macro to convert seconds to nanoseconds.
#define RCUTILS_S_TO_NS(seconds) (seconds * (1000LL * 1000LL * 1000LL))
/// Convenience macro to convert milliseconds to nanoseconds.
#define RCUTILS_MS_TO_NS(milliseconds) (milliseconds * (1000LL * 1000LL))
/// Convenience macro to convert microseconds to nanoseconds.
#define RCUTILS_US_TO_NS(microseconds) (microseconds * 1000LL)

/// Convenience macro to convert nanoseconds to seconds.
#define RCUTILS_NS_TO_S(nanoseconds) (nanoseconds / (1000LL * 1000LL * 1000LL))
/// Convenience macro to convert nanoseconds to milliseconds.
#define RCUTILS_NS_TO_MS(nanoseconds) (nanoseconds / (1000LL * 1000LL))
/// Convenience macro to convert nanoseconds to microseconds.
#define RCUTILS_NS_TO_US(nanoseconds) (nanoseconds / 1000LL)

So RCUTILS_S_TO_NS(1 + 1) is not 2000000000LL as one might expect, but 1000000001LL, because of operator precedence rules. This case is not triggered in rclcpp. I will open a separate pull request in https://github.com/ros2/rcutils for that.

[fujitatomoya]

LGTM

[clalancette]

The change looks good to me.

While a test in rclcpp for this is OK, I'd prefer to have the test here in this package. That way the package that is providing the functionality is also testing it. Can you add a test for the problematic cases here?

[meyerj]

Can you add a test for the problematic cases here?

Done in ab897e4.

The new unit test test_rcutils_time_conversion_macros covers the cases where the macro argument is a sum, and also highlights some other potentially problematic cases because of the limited precision of type double compared to int64_t.

[clalancette]

Nice set of tests, thanks! Will run CI next.

[clalancette]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

For this (and all of the other double checks), it seems to me that instead of casting the result of the macro to int64_t, we should actually change the expected value to be a double. After all, that would more closely test that the output of the macro returns the type we expect, no?

[meyerj]

That is clearly beyond scope of this pull request, I think. Why would you expect the macros to return seconds as double? They don't. They return the same type as their argument, and eventually promote the integer to the width of a long long because of the ll suffix of the constants.

But RCUTILS_MS_TO_S(1) for example returns 0, even if you would cast the result to double afterwards. Calling the macros with a uint64_t triggers a sign-conversion warning.

If you expect certain fixed input and output types, then they should probably be replaced by inline functions or the macros need some additional explicit casts. But that would be a potentially breaking change.

[meyerj]

I am sorry, I associated your comment with the wrong line of code. Clearly, if the argument is of type double, the output can be expected of type double, too, and the cast to int64_t can be omitted.

[clalancette]

I am sorry, I associated your comment with the wrong line of code. Clearly, if the argument is of type double, the output can be expected of type double, too, and the cast to int64_t can be omitted.

Yeah, that's what I was getting at. Sorry if I wasn't clear. Anyway, could you please make that change? Thanks.

[meyerj]

My thought was that the macro might be used to assign to an int64_t, like

double seconds = 42.5;
int64_t nanoseconds = RCUTILS_S_TO_NS(seconds);

But I could not find any usage of these macros with a floating point argument and this code snippet triggers a -Wconversion compiler warning, so probably they were just never meant for that purpose.

[clalancette]

New CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[meyerj]

New CI:

• Linux
• Linux-aarch64
• macOS
• Windows

Are the test failures on Windows related to this patch? I do not see how.

I am afraid that I cannot follow-up on this anymore. Creating and amending the unit tests for such a small and trivial patch as adding parentheses for macro arguments already cost too much time, and the multi-platform support makes it hard to test locally before applying changes (or I am not aware of the procedures yet).

[clalancette]

Are the test failures on Windows related to this patch? I do not see how.

Yeah, I don't see how either. We have some known issues with services in ROS 2, so I'm going to chalk it up to that. Thanks for iterating, I'm going to go ahead and merge.