-
-
Notifications
You must be signed in to change notification settings - Fork 747
cstring.d: add overflow checks #4718
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
Conversation
|
LGTM. This one is a bit more complex, so I'll let another set of eyes look before merging. The others were trivial. I'm sure the code is fine, I'm just wondering if there's ways to avoid some of these overflow checks. The +1 check inside the case where it's on the stack, maybe could be done simpler. |
|
The trouble with things like "in this case we don't need an overflow check because of obscure XXX" is when the code is changed, XXX may inadvertently no longer hold true, and the overflow risk returns. Most of these allocations are not on the hot path, so an extra check is not a problem. Furthermore, at some point I want to integrate the check functions into the compiler so the code cost for them will become nearly negligible. |
|
What I mean is that things like: bool overflow;
auto nbytes = addu(someval, 1, overflow);
if(overflow) assert(0);Can be done like: if(someval == someval.max) assert(0);
auto nbytes = someval + 1; // or leave existing codeI mean, it's fine the way you have it, and I would be fine merging it, but when you are looking at adding or multiplying constants, it seems there can be more efficient ways to check. You are probably right that it's not worth the trouble. I agree that integrating into the compiler and then having access to hardware flags makes this even less invasive (checking the carry flag may be enough). Complex calculations may prove more difficult since the flags are going to be overwritten. |
3f6ea64 to
19a5fcd
Compare
|
@schveiguy Implemented your suggestion. |
| if (res_is_onstack) | ||
| { | ||
| if (newlen <= strLength) | ||
| { |
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.
Missing closing brace for this
|
Sorry to be Debbie Downer on this but this initiative seems a waste to me.
|
No description provided.