-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make static_string trivially copyable #57
base: develop
Are you sure you want to change the base?
Conversation
testTypeTraits() | ||
{ | ||
{ | ||
using S = static_string<1>; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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. :)
That sounds great. |
How was it not already trivially copyable... |
@vinniefalco because we use character traits |
My only reservation with this PR is that this would result in all bytes being copied (regardless of |
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. |
Ah, I see, that's the same issue raised in Folly's answer to the tradeoff was essentially equivalent to:
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. :) |
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.) |
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 doesnoexcept
, 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.