Think this should check for < 0

You're right, but maybe <= 0? If it returns zero then it means it wrote no bytes, which also seems like an issue.

agreed!

I don't think I can test this case, since I get no feedback (other than output to stderr) when this occurs (there's no return code from the function).

I fixed the comparisons here: 7f1a17b

I guess users might use {seconds}.{nanoseconds} but then get confused by the variable nature of digits of the nanoseconds part. I think an additional placeholder is desired / necessary which pads the output with leasing zeros to a fixed length of 9 digits.

I think you're confusing the purpose of {nanoseconds}, it's the total timestamp in nanoseconds, not the nanoseconds part of time. {seconds} is floating point time. So, {seconds}.{nanoseconds} doesn't really make any sense. See the documentation:

https://github.com/ros2/rcutils/pull/85/files#diff-f1d39d4e841937fde043a8e49b7e6c2aR62

Oh, got it now. Thanks for clarifying.

that was how I also first interpreted it (as sec.nanosec), so if it's happened to a few people maybe there's a way we can rename it to avoid confusion. We could just do time like in ROS 1 and additionally time_ns? Then you'd be less likely to think {time}.{time_ns}, but 'time' is not as descriptive as we'd like moving forward..

It's a fair point, however, that the padding is never considered. So on the floating point on the seconds may not be consistent from machine to machine. I'll see what I can do about that, as ideally it would be similar.

I'm actually writing the missing test right now, I so I'll think if a fixed padding makes sense in either case.

It doesn't need to be concise I don't think, we could use timestamp and timestamp_as_nanoseconds.

@dhood and I talked about it offline and we decided on time and time_as_nanoseconds, using time rather than timestamp to be closer to what ROS 1 does, and using _as_nanoseconds to be maximumly specific without worrying about verbosity too much because we expect it to not be used that much in favor of the floating point seconds version.

I understand that the motivation for this is so that positive and negative outputs are aligned, but isn't that only going to be necessary in like 0.1% of use cases? I think the small use case of swapping between negative and positive timestamps and wanting them to be aligned doesn't warrant the extraneous space for 99.9% of use cases (I might be missing something though). Having the default output be [ 123.456] is surprising to me.

I understand that the motivation for this is so that positive and negative outputs are aligned, but isn't that only going to be necessary in like 0.1% of use cases?

That's right, I had reservations about it too, but given I made these changes in the first place in order to try and keep a fixed width I was placing extreme value on never having a different string length. However, the case pointed out for floats (variable mantissa) is far more likely and the case I'm optimizing for here should honestly never happen (that's why I originally made the timepoint unsigned 😛).

I can change this.

+1 for having the default misaligned for positive/negative numbers, if the misalignment bothers someone they can add an option for it 😄 I know we pad in some cases in ros1 e.g. [ERROR] vs [ INFO] but those are much more likely.

this doc line is copypasta (and below)

you missed this comment @wjwwood

Oops, 94dcf14 😄

If str_size was specified as 1, the snprintf's return value will be 0 from my understanding ("The number of characters that would have been written if n had been sufficiently large, not counting the terminating null character."). I don't think we should error in that case anymore. However, there's a test below for str_size=1 that expects RCUTILS_RET_OK, is that test passing?

All of the tests are passing (and running 😄) from what I can tell. As well as on Windows where the snprintf like functions some times behave slightly differently.

I think that it will return 21 or whatever it would have been, regardless of the value of str_size, rather than returning how many it actually wrote based on truncating due to str_size being smaller than the result.

ohhh, I see. it will successfully put (0 chars + NUL) but won't return 0. good good, this clears it up, thanks

\s instead of will also match other whitespace characters which seems unnecessarily loose. Was there a reason for not just using a space?

Nope, I can change that and rerun the tests.

no need now I was mostly just curious if there was regex trickery going on

leftover 😄

How is this "recommended minimum size" relevant in the docblock? Isn't is sufficient to document the exact max size necessary? Also why 32?

Same below.

Because the usable block size on my machine is 16 bytes, and 32 is the smallest multiple of that in which the exact size of this string fits. Also, it gives us a little flexibility for the future, e.g. if we wanted to add + to the front of the time to distinguish it from -. Not saying we will, but if when you ask for 21 characters malloc gives you 32 anyways...

This is the program I used to check:

#include <cassert>
#include <cstdio>

#if __linux__
#include <malloc.h>
#else
#include <malloc/malloc.h>
#endif

int main()
{
  uint8_t * bytes = (uint8_t *)malloc(1);
  assert(bytes);
#if __linux__
  printf("%zu\n", malloc_usable_size(bytes));
#else
  printf("%zu\n", malloc_size(bytes));
#endif
  return 0;
}

I didn't try it on Windows.