-
-
Notifications
You must be signed in to change notification settings - Fork 606
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
Fix Issue 20842 - Structs with disabled default/copy ctors can't be i… #11159
Conversation
Thanks for your pull request and interest in making D better, @pbackus! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#11159" |
src/dmd/dstruct.d
Outdated
|
||
if (ctor) | ||
{ | ||
auto v = new HasNonDisabledCtorVisitor(); |
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.
Use
scope v = new HasNonDisabledCtorVisitor();
src/dmd/dstruct.d
Outdated
} | ||
} | ||
|
||
if (ctor) |
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.
if (!ctor)
return false;
static extern (C++) class HasNonDisabledCtorVisitor : Visitor
{
...
}
scope v = new HasNonDisabledCtorVisitor();
ctor.accept(v);
return v.result;
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.
Thanks a lot! This bug is indeed very annoying.
Two things:
- Can you use
scope
instead ofauto
for the visitor ?auto
leads to GC allocation, whilescope
means it will be class allocated, so it matters quite a bit. - Can you add similar tests for
union
?
The other comments are purely stylistic.
src/dmd/dstruct.d
Outdated
this() | ||
{ | ||
result = false; | ||
} |
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's the default value, no need to initialize it
src/dmd/dstruct.d
Outdated
if (ctor) | ||
{ | ||
auto v = new HasNonDisabledCtorVisitor(); | ||
ctor.accept(v); | ||
return v.result; | ||
} | ||
else | ||
{ | ||
return false; | ||
} |
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.
if (ctor) | |
{ | |
auto v = new HasNonDisabledCtorVisitor(); | |
ctor.accept(v); | |
return v.result; | |
} | |
else | |
{ | |
return false; | |
} | |
if (!ctor) | |
return false; | |
scope v = new HasNonDisabledCtorVisitor(); | |
ctor.accept(v); | |
return v.result; |
In general, we prefer to use early returns like this, to get the obvious case "out of the way" and simplify the flow, as well as reduce the amount of unneeded indentation.
src/dmd/dstruct.d
Outdated
if (!(cd.storage_class & STC.disable)) | ||
{ | ||
result = true; | ||
} |
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.
Here and in the OverloadSet
overload, there is a 100% overhead in vertical space because of the optional {}
.
Personally not found of it, but if you feel strongly about keeping it, it's fine.
@Geod24 @thewilsonator Thanks for the feedback; I've made the changes you suggested. |
…nitialized A struct must now have at least one non-@disabled constructor to be ineligible for brace initialization.
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.
Nice, LGTM
CI failures appear to be spurious (timeouts). |
This PR has introduced a regression: https://issues.dlang.org/show_bug.cgi?id=21547 . I have looked at this PR and the problem is that the visitor does not check for all overloads. It simply looks at the first ctor declaration. |
…nitialized
A struct must now have at least one non-
@disable
d constructor to beineligible for brace initialization.