Don't use an additional buffer in the arithmetic conversions #67

gennaroprota · 2025-10-02T17:08:39Z

In the case of the floating point conversions, this effectively avoids a copy of the buffer contents. In the case of the integer conversions, it doesn't eliminate the copy, but still removes the extra buffer.

Flamefire · 2025-10-03T10:06:29Z

Maybe merge #64 first to let the full CI run, I disabled C++26 testing just now to let it pass until #66 is resolved

Copilot

Pull Request Overview

This PR optimizes arithmetic conversion functions by eliminating the use of intermediate character buffers. The changes directly write formatted values into the result string objects instead of using temporary buffers and then copying the data.

Replaced temporary char/wchar_t arrays with direct manipulation of static_string objects
Added friend function declarations to enable access to private members
Modified floating-point conversions to write directly to result string data

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-03T17:29:52Z

include/boost/static_string/static_string.hpp

+  static_string<N> result;
+  result.set_size(N);
+  const auto digits_end = result.end();
  const auto digits_begin = integer_to_string<std::char_traits<char>, Integer>(
    digits_end, value, std::is_signed<Integer>{});
-  return static_string<N>(digits_begin, std::distance(digits_begin, digits_end));
+  result.set_size(std::distance(digits_begin, digits_end));
+  std::char_traits<char>::move(result.data(), digits_begin, result.size());


Setting the size to N initially and then immediately changing it creates an inefficient pattern. The string is first sized to N, then resized based on the actual digits needed. This could lead to unnecessary initialization of the buffer contents.

Copilot · 2025-10-03T17:29:52Z

include/boost/static_string/static_string.hpp

  // we know that a formatting error will not occur, so
  // we assume that the result is always positive
-  if (std::size_t(std::snprintf(buffer, N + 1, "%f", value)) > N)
+  std::size_t length = std::snprintf(result.data(), N + 1, "%f", value);


Writing N + 1 characters to result.data() when the result string has capacity N is a buffer overflow. The snprintf call should use N + 1 for the buffer size only if result has been allocated with that capacity.

Copilot · 2025-10-03T17:29:52Z

include/boost/static_string/static_string.hpp

  // we know that a formatting error will not occur, so
  // we assume that the result is always positive
-  if (std::size_t(std::snprintf(buffer, N + 1, "%Lf", value)) > N)
+  std::size_t length = std::snprintf(result.data(), N + 1, "%Lf", value);


Same buffer overflow issue as with the double version - writing N + 1 characters to a buffer with capacity N.

Copilot · 2025-10-03T17:29:52Z

include/boost/static_string/static_string.hpp

-  const long long num_written =
-    std::swprintf(buffer, N + 1, L"%f", value);
+  long long num_written =
+    std::swprintf(result.data(), N + 1, L"%f", value);


Buffer overflow issue - attempting to write N + 1 wide characters to a buffer with capacity N.

Copilot · 2025-10-03T17:29:53Z

include/boost/static_string/static_string.hpp

-  const long long num_written =
-    std::swprintf(buffer, N + 1, L"%Lf", value);
+  long long num_written =
+    std::swprintf(result.data(), N + 1, L"%Lf", value);


Buffer overflow issue - attempting to write N + 1 wide characters to a buffer with capacity N.

Flamefire · 2025-10-04T08:57:28Z

include/boost/static_string/static_string.hpp

  return last;
 }

+template<std::size_t N, typename Integer>


Why are those declarations required after the definitions above?

Yes, those are not needed, thanks.

Flamefire · 2025-10-04T08:58:25Z

include/boost/static_string/static_string.hpp

  template<std::size_t, class, class>
  friend class basic_static_string;
+
+  template<std::size_t P, typename Integer>


They don't use any private properties, do they? So no need for friends

They use set_size(), which is private in basic_static_string due to private inheritance.

Adding all these friend functions for to_string implementations feels odd. Ideally, users should be able to create their own similar utility functions with comparable performance, but they can't make their functions friends of static_string. Would adding a resize_and_overwrite member function remove the need for those friends?

OK. I've added resize_and_overwrite() and implemented the arithmetic conversions in terms of it. Please have a look.

Ah, it was just a suggestion, but you've already implemented it, and it seems to work.
@pdimov, does adding resize_and_overwrite() to static_string make sense? Context: Gennaro wants to rewrite the to_static_string functions and eliminate the use of a temporary buffer:

static_string/include/boost/static_string/static_string.hpp

Lines 552 to 556 in 0d255f7

char buffer[N];

const auto digits_end = std::end(buffer);

const auto digits_begin = integer_to_string<std::char_traits<char>, Integer>(

digits_end, value, std::is_signed<Integer>{});

return static_string<N>(digits_begin, std::distance(digits_begin, digits_end));

This enables RVO and produces more efficient assembly: https://godbolt.org/z/bbesGEPj8

I suppose it does.

Flamefire · 2025-10-06T11:51:21Z

include/boost/static_string/static_string.hpp

-  char buffer[N];
-  const auto digits_end = std::end(buffer);
+  static_string<N> result;
+  result.set_size(N);


This is redundant here, isn't it?

Suggested change

result.set_size(N);

The first set_size() is not redundant with the constructor, because the constructor creates a string with size zero. I could avoid it by writing:

const auto digits_end = result.begin() + N;

but I don't see any way around the second set_size().

Ah yes, I missed the use of result.end(), so it is fine then although a bit confusing to some (like me)

To me the most readable approach would be const auto digits_end = result.data() + N; as it highlights the buffer use and mirrors the use in the move below.

Flamefire · 2025-10-06T11:51:39Z

include/boost/static_string/static_string.hpp

-  wchar_t buffer[N];
-  const auto digits_end = std::end(buffer);
+  static_wstring<N> result;
+  result.set_size(N);


See above

Suggested change

result.set_size(N);

Flamefire · 2025-10-06T15:01:27Z

include/boost/static_string/static_string.hpp

+  result.set_size(N);
+  const auto digits_end = result.end();


Suggested change

result.set_size(N);

const auto digits_end = result.end();

const auto digits_end = result.data() + N;

ashtum · 2025-10-06T19:43:47Z

this effectively avoids a copy of the buffer contents. In the case of the integer conversions, it doesn't eliminate the copy, but still removes the extra buffer.

Nice, it also enables RVO and produces more efficient assembly because it skips the call to the constructor that is not noexcept: https://godbolt.org/z/bbesGEPj8

ashtum · 2025-10-07T16:34:32Z

include/boost/static_string/static_string.hpp

+  Operation op)
+{
+  if (n > max_size()) {
+    detail::throw_exception<std::length_error>("n > max_size() in resize_and_overwrite()");


I believe the message "n > max_size()" (identical to what's thrown in resize()) is sufficient. there's no need to proliferate string literals.

Is it sufficient? It gives no clue to the user about where the problem occurred.

I believe detail::throw_exception uses BOOST_THROW_EXCEPTION under the hood, which would also print the source location. But generally, the exception message doesn't need to include the current function name, "n > max_size()" is descriptive enough for this kind of error message in the code.

BOOST_THROW_EXCEPTION, if used inside detail::throw_exception, would emit the source location of detail::throw_exception.

You probably want boost::throw_with_location here, or the two argument overload of boost::throw_exception, so that the source location of the throw is captured, not that of detail::throw_exception.

@gennaroprota, it might be more convenient to create an issue for this and address it in a separate PR once you’re done with the current one.

What if BOOST_STATIC_STRING_STANDALONE is defined?

If this configuration is to still be supported (is it?) you'd probably need to define BOOST_STATIC_STRING_CURRENT_LOCATION and then use detail::throw_exception( E(...), BOOST_STATIC_STRING_CURRENT_LOCATION ) instead of boost::throw_exception( E(...), BOOST_CURRENT_LOCATION ).

Something better might also be possible, I'm not sure.

If this configuration is to still be supported (is it?)

I'd be happy to remove the support. The less conditional compilation, the better.

We should probably ask on the list.

ashtum · 2025-10-07T16:46:04Z

test/static_string.cpp

 }

+void
+testResizeAndOverwrite()


It might be better to split this PR into two commits: the first adds the resize_and_overwrite function, and the second improves the performance of the to_string implementations.

Flamefire · 2025-10-07T17:12:55Z

include/boost/static_string/static_string.hpp

+  /**
+      Resize the string and overwrite its contents.
+
+      Resizes the string to contain `n` characters, and uses the


I'm not sure that is useful, the size passed here doesn't really matter, does it? Maybe a write_and_resize(op) is more useful? Or is there any realistic use case for this param being less than N?

That's how the function is specified for std::string.

For static_string specifically the only thing the passed size does is throw if it's too big.

Ok, makes sense

In the case of the floating point conversions, this effectively avoids a copy of the buffer contents. In the case of the integer conversions, it doesn't eliminate the copy, but still removes the extra buffer. This fixes issue #65.

Reason: This is in preparation of the next commit. See its commit message.

Reason: Performing the conversions without accessing private members, providing a model for users to implement their own with comparable efficiency.

gennaroprota requested a review from vinniefalco October 2, 2025 17:08

gennaroprota force-pushed the feature/dont_use_an_additional_buffer_in_the_arithmetic_conversions branch 2 times, most recently from aa894b1 to 6945249 Compare October 2, 2025 17:14

vinniefalco requested a review from Copilot October 3, 2025 17:28

Copilot AI reviewed Oct 3, 2025

View reviewed changes

Flamefire reviewed Oct 4, 2025

View reviewed changes

Flamefire reviewed Oct 6, 2025

View reviewed changes

gennaroprota force-pushed the feature/dont_use_an_additional_buffer_in_the_arithmetic_conversions branch from 6945249 to c103f63 Compare October 6, 2025 14:49

Flamefire reviewed Oct 6, 2025

View reviewed changes

gennaroprota force-pushed the feature/dont_use_an_additional_buffer_in_the_arithmetic_conversions branch 3 times, most recently from 31d70c8 to 15a1e18 Compare October 6, 2025 15:19

ashtum reviewed Oct 7, 2025

View reviewed changes

Flamefire reviewed Oct 7, 2025

View reviewed changes

gennaroprota force-pushed the feature/dont_use_an_additional_buffer_in_the_arithmetic_conversions branch 3 times, most recently from 07d2e93 to 4796985 Compare October 9, 2025 15:39

gennaroprota added 3 commits October 9, 2025 17:45

Add a resize_and_overwrite() member

300781d

Reason: This is in preparation of the next commit. See its commit message.

Implement the arithmetic conversions in terms of resize_and_overwrite()

9c5d694

Reason: Performing the conversions without accessing private members, providing a model for users to implement their own with comparable efficiency.

gennaroprota force-pushed the feature/dont_use_an_additional_buffer_in_the_arithmetic_conversions branch from 4796985 to 9c5d694 Compare October 9, 2025 15:47

gennaroprota merged commit 9c5d694 into develop Oct 9, 2025
1 check was pending

	char buffer[N];
	const auto digits_end = std::end(buffer);
	const auto digits_begin = integer_to_string<std::char_traits<char>, Integer>(
	digits_end, value, std::is_signed<Integer>{});
	return static_string<N>(digits_begin, std::distance(digits_begin, digits_end));

	result.set_size(N);
	const auto digits_end = result.end();
	const auto digits_end = result.data() + N;

Don't use an additional buffer in the arithmetic conversions #67

Don't use an additional buffer in the arithmetic conversions #67

Conversation

gennaroprota commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Flamefire commented Oct 3, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flamefire Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashtum commented Oct 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gennaroprota commented Oct 2, 2025 •

edited

Loading

Flamefire Oct 6, 2025 •

edited

Loading