Fix Issue 19754 - cast() sometimes yields lvalue, sometimes yields rvalue#9505
Fix Issue 19754 - cast() sometimes yields lvalue, sometimes yields rvalue#9505dlang-bot merged 1 commit intodlang:masterfrom
Conversation
|
Thanks for your pull request and interest in making D better, @RazvanN7! 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 fetch digger
dub run digger -- build "master + dmd#9505" |
RazvanN7
force-pushed
the
Alt_19754
branch
2 times, most recently
from
231fef0 to
6c2d367
Compare
| /* | ||
| TEST_OUTPUT: | ||
| --- | ||
| fail_compilation/ice14929.d(45): Error: `cast(Node)(*this.current).items[this.index]` is not an lvalue and cannot be modified |
There was a problem hiding this comment.
this was casting a const(Node) to a Node.
| /* | ||
| TEST_OUTPUT: | ||
| --- | ||
| fail_compilation/diag4596.d(15): Error: `this` is not an lvalue and cannot be modified |
There was a problem hiding this comment.
The branches of the ?: operator may return different things, e.g. an rvalue and an rvalue; this way the error is printed depending on the branch
src/dmd/expression.d
Outdated
| override bool isLvalue() | ||
| { | ||
| //printf("e1.type = %s, to.type = %s\n", e1.type.toChars(), to.toChars()); | ||
| return e1.type.mutableOf().unSharedOf() == to.mutableOf().unSharedOf(); |
There was a problem hiding this comment.
Tests look good now, but shouldn't this use implicitConvTo to satisfy "cast(U) expressions applied to lvalues of type T when T* is implicitly convertible to U*;"?
|
As discussed with @RazvanN7 : the way slices with constant limits are handled is an irregularity in the language. For the time being I suggest we support and document it. |
|
I put a link here, hopefully, the pr will fix it https://issues.dlang.org/show_bug.cgi?id=19793 |
RazvanN7
force-pushed
the
Alt_19754
branch
2 times, most recently
from
ce82a25 to
87a21d3
Compare
Codecov Report
@@ Coverage Diff @@
## master #9505 +/- ##
==========================================
- Coverage 84.86% 84.86% -0.01%
==========================================
Files 143 143
Lines 74044 74056 +12
==========================================
+ Hits 62835 62845 +10
- Misses 11209 11211 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #9505 +/- ##
==========================================
- Coverage 84.86% 84.86% -0.01%
==========================================
Files 143 143
Lines 74044 74056 +12
==========================================
+ Hits 62835 62845 +10
- Misses 11209 11211 +2
Continue to review full report at Codecov.
|
|
@andralex all tests green now |
|
Great, thanks! |
|
The testcase ( void main()
{
shared shared(int) x = 0;
x = 5;
const const(int) x1 = 0;
0 = 5; // wtf?
assert(false);
immutable immutable(int) x2 = 0;
0 = 5; // :)
assert(false);
return 0;
}While DMD seems to swallow this (but assertions failing as expected), LDC doesn't allow/expect assigning to an IntegerExp/integer literal (!). |
Compiling the code with DMD fails with: |
|
Glue layer/backend swallows this codegen AST ( |
|
I tried Lines 8769 to 8770 in bc6bb99 but that's not enough. |
Fix Issue 19754 - Re-apply #9505 and fix it, making isLvalue() logic more restrictive wrt. literals merged-on-behalf-of: unknown
9 participants
The check for modifiable lvalues was done after an optimization pass that did some const folding which was not performed in the case of
shared. This led to accepting invalid code in the case of castedshareddata and confusing error messages for other qualified data which was cast to unqualified.Later edit: I added the missing bits so that the code now behaves according to the spec; that is:
cast()andcast($qualifier)returns an lvalue.