-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix udl_compiled_string with non-byte chars (e.g. wchar) #2242
Fix udl_compiled_string with non-byte chars (e.g. wchar) #2242
Conversation
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.
This is totally fine. I did exactly the same in a different context outside of UDLs.
constexpr detail::udl_compiled_string<remove_cvref_t<decltype(Str.data[0])>, | ||
sizeof(Str.data), Str> | ||
constexpr detail::udl_compiled_string< | ||
remove_cvref_t<decltype(Str.data[0])>, |
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.
Can we move remove_cvref_t<decltype(Str.data[0])>
to a default template parameter to avoid repetition? i.e.
template <detail::fixed_string Str, Char = remove_cvref_t<decltype(Str.data[0])>>
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.
Looks like it's not possible because compilers expect to see certain template arguments - https://godbolt.org/z/6odY6E78h
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.
Can we at least do typename Str::char_type
?
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.
How about value_type
as in basic_string<>::value_type
?
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, value_type
is a more appropriate.
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.
Don't forget that Str
is not a type, it's an object since it's passed as a non-type template parameter, so we have these options here:
<remove_cvref_t<decltype(Str.data[0])>, sizeof(Str.data) / sizeof(decltype(Str.data[0])), Str>
vs.
<typename decltype(Str)::value_type, sizeof(Str.data) / sizeof(typename decltype(Str)::value_type), Str>
and they both are not great.
But the size of fixed_string
can be exposed by a static member or function, like so:
<typename decltype(Str)::value_type, decltype(Str)::size, Str>
Just let me know if this approach is preferable.
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.
I think it's fine as is then. Thanks for the fix.
Without this fix,
udl_compiled_string
works only for char types that have a 1-byte size.