fix Issue 17722 - Wrong diagnostic using __traits(compiles, e1 && e2) expressions #7067
Conversation
src/ddmd/expressionsem.d
Outdated
| if (f1 || f2) | ||
| { | ||
| result = new ErrorExp(); | ||
| return; | ||
| } | ||
|
|
||
| if (exp.e2.type.ty == Tvoid) | ||
| if (e2x.type.ty == Tvoid) |
There was a problem hiding this comment.
By assigning to exp.type here, the type gets disconnected from the operands, putting exp into an invalid state. A better approach is to assign to a t here, and assign exp.type = t; when the operands are assigned.
There was a problem hiding this comment.
Hmm, you mean that if the first time round the caller ignores the propagated error and runs semantic on this expression again?
There was a problem hiding this comment.
That is to say, setting type means that you are saying this expression has completed its semantic, when infact it could instead return error (in which case, semantic did not complete).
There was a problem hiding this comment.
That's right. Even if it doesn't, it looks suspicious, which will cause extra work for anyone trying to understand the code.
src/ddmd/expressionsem.d
Outdated
| exp.type = Type.tvoid; | ||
| else | ||
| { | ||
| exp.e2 = exp.e2.toBoolean(sc); | ||
| e2x = e2x.toBoolean(sc); |
src/ddmd/expressionsem.d
Outdated
| return; | ||
| } | ||
|
|
||
| exp.e1 = e1x; | ||
| exp.e2 = e2x; |
src/ddmd/expressionsem.d
Outdated
| if (f1 || f2) | ||
| { | ||
| result = new ErrorExp(); | ||
| return; | ||
| } | ||
|
|
||
| if (exp.e2.type.ty == Tvoid) | ||
| if (e2x.type.ty == Tvoid) |
|
Actually in this case, as the logic in-between setting |
|
When fixing a bugzilla issue, please put the PR URL in the bugzilla issue, so somebody else doesn't waste time trying to fix it because he's ignorant there is an extant fix already. |
|
I still think that the cross reference should be handled by the dlang bot/whatever github hook posts messages to bugzilla on commits. |
|
Fixed the wrong filename in the test output. |
|
Tests pass. |
| @@ -0,0 +1,13 @@ | |||
| /* TEST_OUTPUT: | |||
There was a problem hiding this comment.
Please merge these two test files into one. It'll consume half the resources of the autotester that way.
There was a problem hiding this comment.
It's kept as two separate because the compiler doesn't issue an error for both when together in the same file.
|
You'd have had half the work to do if you'd pulled #7069 :-) |
I'd still end up doing double work, because then I'd have to rebase this PR. |
Manipulates
e1ande2in a locale1xande2xvar respectively, and assigns them back if semantic was successful.This is the same as what other binary expressions do via
binSemantic/binSemanticProp.