Add string_view and string conversion operators and functions to cstring #4676
Conversation
vlstill
added
the
core
| @@ -165,6 +166,12 @@ class cstring { | |||
| const char *c_str() const { return str; } | |||
| operator const char *() const { return str; } | |||
|
|
|||
| std::string string() const { return std::string(str); } | |||
There was a problem hiding this comment.
Could this mess with our C++20 conversion efforts? I remember we ran into some issues related to ambiguous string conversion.
There was a problem hiding this comment.
It should not as all the added conversions are explicit. Ideally we would make the existing conversions explicit too, but that is probably a no-go (I've tried looking into making cstring -> IR::ID explicit and the number of changes that would be required was HUGE).
As for C++20, I think we will probably have to name all the combinations of comparison operators for the C++20 to work (the exact match has always precedence over cast, so we would need all comparsions between cstring, IR::ID at least (std::string and std::string_view too). Luckily defining them in just one direction should be enough, but unluckily the <=> is not in GCC 9 but in GCC 10, so that might be a problem. We would also need to make sure there is no possibility to get clashes by flipping argument order. I haven't tried that yet.
There was a problem hiding this comment.
Just bike-shedding: won't it be better to name str?
There was a problem hiding this comment.
str is the name of the (private) member that holds the actual const char * data. So that would require renaming it. Also I don't like .sv() as a .string_view() and if we have string_view() it is more consistent to have .string(). Naming is hard...
| @@ -203,8 +203,7 @@ cstring cstring::before(const char *at) const { return substr(0, at - str); } | |||
|
|
|||
| cstring cstring::substr(size_t start, size_t length) const { | |||
| if (size() <= start) return cstring::empty; | |||
| std::string s = str; | |||
| return s.substr(start, length); | |||
| return cstring(string_view().substr(start, length)); | |||
There was a problem hiding this comment.
Should this maybe return a string_view and have a constructor from a string_view? Not sure if that would break anything.
There was a problem hiding this comment.
I was thinking about this. I'll try to take a look at that later, but I am not very happy about the idea of introducing a new implicit cast to cstring and without that I'm quite sure this would break something.
| explicit operator std::string() const { return string(); } | ||
|
|
||
| std::string_view string_view() const { return std::string_view(str); } | ||
| explicit operator std::string_view() const { return string_view(); } |
There was a problem hiding this comment.
Thinking more about it... What if we do the opposite?
- Conversion to
string_viewshould be implicit - Construction from
const char*should be explicit
Here is the rationale: currently it is not possible to have a string_view function argument if the function is assumed to accept both cstring and const char*. E.g. the following:
void foo(cstring);
void foo(string_view);
foo("bar")Is ambiguous as cstring could be constructed from char* implicitly, string_view could be constructed from char* implicitly, but we cannot drop the cstring overload as cstring cannot be implicitly converted to string_view.
Certainly, there are lots of examples in codebase that do e.g.
cstring getName() const { return "Foo"; }that will be broken. But IMO these should be all fixed to cstring::literal call and explicit conversion to cstring should emphasize that this is not a cheap operation (involving strlen + map lookup).
The
cstringtype is used a lot in the codebase as it is the type of names in the IR. It is, however not a good idea to perform operations oncstringunless the result of the operations is intended to be persistent. In these cases, the operations should be rather performed onstd::stringorstd::string_view(based on what kind of operations is desired). Note that creating astd::string_viewfrom acstringis currently always safe ascstringis persistent.This PR adds explicit operators for conversion to both string and string_view as well as functions which serve the same purpose. I've opted to add these too as I find it syntactically more pleasing to do something like
dec->name.name.string()thanstd::string(dec->name.name).