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
add size() checks to declaration.d #6065
Conversation
635c46e
to
3db6b13
Compare
Current coverage is 81.95% (diff: 100%)@@ master #6065 diff @@
==========================================
Files 96 96
Lines 55797 55776 -21
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 48774 45709 -3065
- Misses 7023 10067 +3044
Partials 0 0
|
Huh? |
3db6b13
to
f1ec6b8
Compare
const vsz = v.type.size(); | ||
const tsz = type.size(); | ||
assert(vsz != SIZE_INVALID && tsz != SIZE_INVALID); | ||
return ( offset < v.offset + vsz && |
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.
odd spaces after paren
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.
Also: return (expr);
is an unrecommended (we should use a style rule to automatically reject it), use return expr;
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.
The spaces are to line things up with the next line. I removed the parens.
f1ec6b8
to
a4a2eee
Compare
@@ -976,7 +976,7 @@ public: | |||
{ | |||
// Bugzilla 13776: Don't use ti->toAlias() to avoid forward reference error | |||
// while printing messages. | |||
TemplateInstance ti = t.sym.parent.isTemplateInstance(); | |||
TemplateInstance ti = t.sym.parent ? t.sym.parent.isTemplateInstance() : null; |
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.
Looks unnecessary.
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.
It is. It crashes otherwise.
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.
It crashes otherwise.
It does indeed
Auto-merge toggled on |
This caused a regression. |
return null; | ||
|
||
const sz = type.size(); | ||
assert(sz != SIZE_INVALID); |
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.
Apparently the sz
can have an invalid size: https://issues.dlang.org/show_bug.cgi?id=17451
Protect against size() failures and divide-by-zero. Not sure if any of these could happen, but added asserts to be sure.