-
Notifications
You must be signed in to change notification settings - Fork 445
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
[RFC] Make cstring constructor from char*
explicit and string_view
conversion implicit
#4688
Comments
Tagging @vlstill @fruffy @ChrisDodd for opinions |
Yes. |
I like the idea of explicit constructor for I've not tried this one I think, but I've tried to make the construction of
So making I've also checked how the standard library does similar conversions and it seems to be similar case as you propose (with the additional problem that the implicit cast in stdlib can introduce dangling reference but
|
@vlstill Yeah. That's lots of work. I already started changes here and there. Some observations:
Maybe something else will occur on my way :) |
So, I just moved a downstream backend over the changes in #4694 It was not that painful. Important story that it uncovered lots of rusty cruft accumulated over the time:
I would say it was definitely worth the cleanup :) |
Some story from dowstream code. It had code like this: void foo(cstring a, cstring b, ...);
void bar() {
..
std::string s = a->toString().c_str();
if (rare_condition)
s += prefix;
foo(s, s);
}
So, just changing the code to: void foo(cstring a, cstring b, ...);
void bar() {
..
cstring s = a->toString();
if (rare_condition)
s += prefix;
foo(s, s);
} yielded a noticeable speedup as no memory allocations and map lookups were performed on most probable hot path. Even on slow path we only pay single price for cstring construction. This overhead went unnoticed due to implicit conversions... |
Thinking more about it... What if we do the opposite?
string_view
should be implicitconst char*
should be explicitHere is the rationale: currently it is not possible to have a
string_view
function argument if the function is assumed to accept bothcstring
andconst char*
. E.g. the following:Is ambiguous as
cstring
could be constructed fromchar*
implicitly,string_view
could be constructed fromchar*
implicitly, but we cannot drop thecstring
overload ascstring
cannot be implicitly converted tostring_view
.Certainly, there are lots of examples in codebase that do e.g.
that will be broken. But IMO these should be all fixed to
cstring::literal
call and explicit conversion tocstring
should emphasize that this is not a cheap operation (involvingstrlen
+ map lookup).Originally posted by @asl in #4676 (comment)
The text was updated successfully, but these errors were encountered: