cstring-related cleanup, switch to std::string_view for some cstring API #4716
Conversation
fruffy
added
core
I would consider splitting these into two separate PRs? At least getting rid of Util::PathName, which is a good thing, is fairly involved. I am not quite clear how many of the changes in this PR are related to the PathName refactoring and how many are related to other cleanups. |
lib/path.h
Outdated
| static PathName empty; | ||
| }; | ||
| // We used to have local implementation. Make this alias for now | ||
| using PathName = std::filesystem::path; |
There was a problem hiding this comment.
If we make this a separate PR I'd consider replacing this alias entirely. I do not see a reason to keep it, really.
Or, add a deprecation notice, if possible.
There was a problem hiding this comment.
std::filesystem::path is quite long to type, in this sense PathName is much shorter even with the namespace. Certainly, this is a bit of bike-shedding :)
But yes, we can get rid of it entirely. Let me split into a separate PR and remove it entirely.
| @@ -1581,6 +1581,7 @@ void P4RuntimeSerializer::serializeP4RuntimeIfRequired(const IR::P4Program *prog | |||
|
|
|||
| void P4RuntimeSerializer::serializeP4RuntimeIfRequired(const P4RuntimeAPI &p4Runtime, | |||
| const CompilerOptions &options) { | |||
| // FIXME: get rid of cstring here | |||
There was a problem hiding this comment.
What happens if you replace it with std::filesystem::path?
There was a problem hiding this comment.
There are more dependencies here including parseFileNames logic, etc. I decided to leave it as-is for now.
|
I submitted #4721 for PathName part. Will rebase this one to have just |
…tions while there.
| @@ -254,17 +254,11 @@ class cstring { | |||
| } | |||
| cstring substr(size_t start, size_t length) const; | |||
| cstring replace(char find, char replace) const; | |||
| cstring replace(cstring find, cstring replace) const; | |||
| cstring replace(std::string_view find, std::string_view replace) const; | |||
There was a problem hiding this comment.
This might be the only breaking change in this PR, right?
There was a problem hiding this comment.
Yes. Though if before explicit-cstring-construction it was like replace("foo", "bar"), then after this PR it will start working again w/o changes :)
There was a problem hiding this comment.
Hmm, I wonder why not return a std::string here. In many cases, we would not need the result to be cstring and if the caller needs it, they can always do that explicitly.
There was a problem hiding this comment.
I checked this case, and there is a mixture of usages. So I decided to re-evaluate this when we'll disable implicit conversion to / from std::string – then I will catch all such places :)
asl
changed the title
std::string_viewfor some cstring API