Superfluous va_copy call in rcutils_char_array_vsprintf

[jrutgeer]

There seems to be a superfluous va_copy call in rcutils_char_array_vsprintf.

I cannot asses whether this has a relevant impact wrt logging perfomance, but given recent PR about optimization #381 I assume it's worth mentioning to those who know better than me. :-)

If I understand correctly, va_copy copies *args into args_clone for rcutils_char_array_vsprintf:

rcutils/src/logging.c

Lines 1313 to 1321 in 17dbbec

	if (RCUTILS_RET_OK == status) {
	va_list args_clone;
	va_copy(args_clone, *args);
	status = rcutils_char_array_vsprintf(&msg_array, format, args_clone);
	if (RCUTILS_RET_OK != status) {
	RCUTILS_SAFE_FWRITE_TO_STDERR_WITH_FORMAT_STRING(
	"Error: rcutils_char_array_vsprintf failed with: %d\n", status);
	}
	va_end(args_clone);

However, rcutils_char_array_vsprintf doc states "The va_list args will be cloned before being used, so a user can safely use it again after calling this function."

This seems indeed correct, as rcutils_char_array_vsprintf passes args to _rcutils_char_array_vsprintf, which has its own va_copy:

rcutils/src/char_array.c

Lines 166 to 198 in 17dbbec

	rcutils_ret_t
	rcutils_char_array_vsprintf(rcutils_char_array_t * char_array, const char * format, va_list args)
	{
	int size = _rcutils_char_array_vsprintf(char_array, format, args);
	if (size < 0) {
	RCUTILS_SET_ERROR_MSG("vsprintf on char array failed");
	return RCUTILS_RET_ERROR;
	}
	size_t new_size = (size_t) size + 1; // with the terminating null byte
	if (new_size > char_array->buffer_capacity) {
	rcutils_ret_t ret = rcutils_char_array_expand_as_needed(char_array, new_size);
	if (ret != RCUTILS_RET_OK) {
	RCUTILS_SET_ERROR_MSG("char array failed to expand");
	return ret;
	}
	if (_rcutils_char_array_vsprintf(char_array, format, args) != size) {
	if (rcutils_char_array_fini(char_array) == RCUTILS_RET_OK) {
	RCUTILS_SET_ERROR_MSG("vsprintf on resized char array failed");
	} else {
	RCUTILS_SET_ERROR_MSG("vsprintf on resized char array failed; clean up failed too");
	}
	return RCUTILS_RET_ERROR;
	}
	}
	char_array->buffer_length = new_size;
	return RCUTILS_RET_OK;
	}

rcutils/src/char_array.c

Lines 151 to 164 in 17dbbec

	static int
	_rcutils_char_array_vsprintf(rcutils_char_array_t * char_array, const char * format, va_list args)
	{
	va_list args_clone;
	va_copy(args_clone, args);
	// when doing size calculation, remember the return value of vsnprintf excludes terminating null
	// byte
	int size = vsnprintf(char_array->buffer, char_array->buffer_capacity, format, args_clone);
	va_end(args_clone);
	return size;
	}

So it might be a simple optimization to simply remove the va_list / va_copy in rcutils_char_array_vsprintf.

[fujitatomoya]

@jrutgeer thanks for the issue and explanation. i believe that makes sense.

[fujitatomoya]

both changes can be backported to humble.