-
-
Notifications
You must be signed in to change notification settings - Fork 608
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 21665 - Void initialization should not be allowed for insta… #12326
Conversation
|
Thanks for your pull request, @WalterBright! 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#12326" |
59d90c8
to
49a3f46
Compare
…nces of struct with invariant
|
Azure pipelines: whatever that means. And buildkite's log just ends with no message. |
| @@ -847,6 +847,12 @@ extern (C++) abstract class AggregateDeclaration : ScopeDsymbol | |||
| return type; | |||
| } | |||
|
|
|||
| // Does this class have an invariant function? | |||
| final bool hasInvariant() | |||
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.
can be const, and all the other hasInvariants() too. Excepted perhaps the one that may cache the information.
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.
Won't work because of the overriding one that can't be const.
|
I'm not convinced that special casing structs with invariants is the right approach. I would argue that either we make void initialization of structs invalid in @safe code altogether or we consider the code in the bug report (which by the way, does not compile currently) as correct. I mean, having the code accepted or rejected based on the existance of invariants seems arbitrary to me. How can using a void initialized struct be @safe? |
|
Given the current spec, void initialization cannot be Void initialization could be In this case, it would make sense to take invariants into account because violating a struct invariant is explicitly specified to result in undefined behavior. It follows that any struct instance which violates its invariant must be considered an unsafe value, and therefore that struct types with invariants could not be |
|
Corresponding spec pull dlang/dlang.org#2990 |
|
We must not be hasty in throwing it under the bus. Having an undefined value is not the same thing as undefined behavior: A struct with an invariant implies only a subset of values of the fields should be possible. Therefore, in @safe code void initializers or overlapping unions of them should not be allowed in @safe code as it violates the invariant constraint. |
|
Should this go through a deprecation cycle? In theory, this has the potential to break code out there, however, one may argue that such code was broken anyway. I, personally, think we could bypass the deprecation in this case, but I'm curios what other people think. |
…nces of struct with invariant
The bug report mentions two problems - the one in the title and when a union field has a struct with an invariant. This fixes both.
The code added is very similar to the implementation and checks for
hasPointers().