-
-
Notifications
You must be signed in to change notification settings - Fork 610
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 20603 - 'cannot use non-constant CTFE pointer in an initializer' in recursive structure with overlap #10882
Conversation
|
Thanks for your pull request and interest in making D better, @dkorpel! 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#10882" |
|
@dkorpel Are you still pursuing this? |
|
Yes. The thing that's blocking me is unfamiliarity with DMD's CTFE engine. There's an explicit test checking that "Pointers must not escape from CTFE" (see test/compilable/interpret3.d) and I'm changing that to only mutable pointers. It looks fine at first glance, but I'm a bit worried I'm overlooking something. |
|
I think that the issue is that the CTFE engine rewrites calls to the constructor without understanding that some fields are overlapping and doing so it ends up bypassing the call to the constructor. I looked into Issue 15938 which seems to be another manifestation of this fact. This PR seems to fix the symptom but not the real cause of the issue. |
Do you still think this PR is an improvement or should it be superseded? It's not just about the union code, the simplest manifestation is the first two lines of the test case: enum immutable(int)* x = new int(3);
enum const(int)* y = new int(5);Currently gives: While the equivalent array case works. enum immutable(int)[] a = new int[10];
enum const(int)[] b = new int[10]; |
| } | ||
|
|
||
| Toq ptrRet(bool b) | ||
| { | ||
| string x = "abc"; | ||
| char[] x = "abc".dup; |
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.
Why is this change required?
Nvm, I didn't see the traits(compiles) below.
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.
This change does make sense on its own. Seeing the union, I instantly thought about the other issue.
The function
hasNonConstPointerswas introduced in #102 (commit14d37a1)
Despite the name, it notably seems to not take const/immutable into account. (Though I'm not sure transitive const already was a thing back in 2011).
This PR does not fix issue 11268 because this still fails:
However, instead of failing in
initsem.d, it fails intodt.dwherevisitAddr(AddrExp e)only supports aStructLiteralExp, not aDotVarExplike above.A
DotVarExpcould be supported in that function, but maybe theDotVarExpshould be dissected at an earlier stage. I think arrays initializers work this way: all array elements are visited separately instead of the entire array, which is why the 'non-constant CTFE pointer' error was not raised when taking the address of an element.