-
Notifications
You must be signed in to change notification settings - Fork 441
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
Initialize cstrings with the empty string. #4359
Conversation
11dfc9e
to
efa5b21
Compare
efa5b21
to
34cca86
Compare
I am not here to guide a technical discussion on the contents of the cstring class, only to point out what looks like the main relevant history that I could find in the commit logs. The discussion on this PR from Sep 2018 includes at least @mihaibudiu and @ChrisDodd who weighed in on that round of changes to the cstring implementation in p4c: #1463 The original version seems to be from Mihai in 2016. |
Well, the CI failures expose that there is a LOT of code in the compiler that assumes that cstring can be a nullpointer. Which is a little unpleasant. So this change might be non-trivial. Eventually we may phase out most of cstring in favor of std::string_view. |
We use cstring a lot as an owning object, so it would not be possible to replace it with std::string_view (unless we put heap-allocated pointers that noone but GC is going to collect in them, which seems little weird to me). Side note: overall, if we wanted to take such big refactoring steps, I think there should be greater discussion on what is a good place to invest into. Just on top of my head, I can think about at least 1 really big issue that I would rank higher then |
Looks like it is all (that we can see now) related to generated IR code, so it might not be that hard to change it. I wonder why would anyone use the null cstring as something different then empty one, this looks like a really confusing design to me :-/. |
Related to this, I've been working on code (in https://github.com/ChrisDodd/p4c/tree/cdodd-disablegc) to allow removing the garbage collector and use |
There's a fair amount of code that uses I think if you add |
Well, in that case we could likely replace these with
I also tried to simply forbid the empty constructor. It requires a bit more work but in case where we need the |
This is great, the garbage collector causes quite a few problems nowadays. I will create an issue to track this. |
Closing in favour of #4694. |
This PR is meant to raise a discussion. We stumbled across this in #4357 where an incorrectly initialized cstring caused a bug. This PR changes the default initializer to use the empty string instead of nullpointer.
I am not sure about the motivation about making the default cstring invalid, could be performance related?
Alternatively, we could forbid this empty constructor. This could expose a lot of incorrectly constructed cstrings.