Skip to content

Conversation

@Quuxplusone
Copy link
Contributor

It's not clear to me why this wasn't true from the very beginning, and git grep trivial doesn't enlighten me.
=default requires at least C++11, but then so does noexcept, so that must not be a concern.
Notice that at this point this is an ABI break for static_string; I don't know if you care about ABI breaks.
If you do care about ABI breaks, but would like to mark the type as trivially relocatable (without an ABI break) on compilers that support P1144, let me know and I'll open a separate PR for that.

testTypeTraits()
{
{
using S = static_string<1>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is

using S = static_string<0>;

a thing we need to test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure; updated.

Re "That sounds great" — unclear whether you mean "great" in re "open a new PR for trivially relocatable" (which makes sense only if you don't want to take this one — trivially copyable types are trivially relocatable by definition) or just generally expressing contentment with this PR. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just generally expressing contentment with this PR. :)

@alandefreitas
Copy link
Member

That sounds great.

@vinniefalco
Copy link
Member

How was it not already trivially copyable...

@sdkrystian
Copy link
Member

sdkrystian commented Mar 21, 2024

@vinniefalco because we use character traits

@sdkrystian
Copy link
Member

My only reservation with this PR is that this would result in all bytes being copied (regardless of size()... @pdimov what do you think?

@pdimov
Copy link
Member

pdimov commented Mar 21, 2024

This has always been the tradeoff. I prefer triviality, and memcpy with a constant N is pretty fast these days, but if we envisage static strings being often used with something like N=16384...

Although on second thought, if N is 16384, then one would expect size() to be somewhere in the thousands on average, so copying will be expensive anyway.

I'm pretty sure we touched this during the formal review, but I don't remember what I argued for. :-) Triviality, probably. It just makes more sense, incl. aesthetically.

@Quuxplusone
Copy link
Contributor Author

My only reservation with this PR is that this would result in all bytes being copied (regardless of size())...

This has always been the tradeoff. I prefer triviality, and memcpy with a constant N is pretty fast these days, but if we envisage static strings being often used with something like N=16384...

Ah, I see, that's the same issue raised in folly::small_vector in facebook/folly#1934 (comment) . As I summarized there: we're comparing "trivial copy (i.e. memcpy) size() elements" (saves cache lines, can't fully unroll) against "memcpy capacity() elements" (costs cache lines, can fully unroll).

Folly's answer to the tradeoff was essentially equivalent to:

static_string(const static_string&)
  requires (sizeof(*this) <= hardware_constructive_interference_size / 2)
  = default;
constexpr static_string(const static_string& rhs) noexcept
  { assign(rhs); }

I don't know if you care to do something similar here, or what, but anyway, this is now a tradeoff discussion above my pay grade. :)

@pdimov
Copy link
Member

pdimov commented Mar 21, 2024

They have the advantage of having the thing deployed at scale, so they can measure.

I suppose we can make it trivial for N <= 64, or something like that. Conditional triviality is a pain in C++11 though.

(hardware_interference_size is better left unused because it can lead to nice ABI issues as the warning says.)

@gennaroprota gennaroprota merged commit 37a557d into boostorg:develop Oct 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants