With the changes to switch to require a default handler, there's some fall-out in runnable/sdtor.d.

line 1271:
  if (0) switch(1) A51 a;

runnable/sdtor.d(1271): Error: non-final switch statement must have a default

So, is d (and thus dmd) supposed to deal with that case better?  I assume no since there's no case 1:, and that the test
should be changed to:
  if (0) switch(1) { A51 a; default: }

Of course, the opposite is also interesting in terms of 'in what cases is a actually constructed/destructed':
  if (0) switch(1) { default: A51 a; }

Lastly, now that struct dtors are working, all of those cases should probably be updated to validate that the dtor is actually called.

With that in mind, review this diff.  Particularly the last line of the test, I think the current behavior is wrong.

$ git diff
diff --git a/test/runnable/sdtor.d b/test/runnable/sdtor.d
index 1f95710..45c3867 100644
--- a/test/runnable/sdtor.d
+++ b/test/runnable/sdtor.d
@@ -1249,28 +1249,31 @@ void test50()

 /**********************************/

+int A51_a;
+
 struct A51
 {
-    ~this() { }
+    ~this() { ++A51_a; }
 }

 void test51()
 {
-  while(0) A51 a;
-  if(0) A51 a;
-  if(1){} else A51 a;
-  for(;0;) A51 a;
-  if (1) { A51 a; }
-  if (1) A51 a;
-  if(0) {} else A51 a;
-  if (0) for(A51 a;;) {}
-  if (0) for(;;) A51 a;
-  do A51 a; while(0);
-  if (0) while(1) A51 a;
-  try A51 a; catch(Error e) {}
-  if (0) switch(1) A51 a;
-  final switch(0) A51 a;
-  A51 a; with(a) A51 b;
+  A51_a = 0; while(0) A51 a;                      assert(A51_a == 0);
+  A51_a = 0; if(0) A51 a;                         assert(A51_a == 0);
+  A51_a = 0; if(1){} else A51 a;                  assert(A51_a == 0);
+  A51_a = 0; for(;0;) A51 a;                      assert(A51_a == 0);
+  A51_a = 0; if (1) { A51 a; }                    assert(A51_a == 1);
+  A51_a = 0; if (1) A51 a;                        assert(A51_a == 1);
+  A51_a = 0; if(0) {} else A51 a;                 assert(A51_a == 1);
+  A51_a = 0; if (0) for(A51 a;;) {}               assert(A51_a == 0);
+  A51_a = 0; if (0) for(;;) A51 a;                assert(A51_a == 0);
+  A51_a = 0; do A51 a; while(0);                  assert(A51_a == 1);
+  A51_a = 0; if (0) while(1) A51 a;               assert(A51_a == 0);
+  A51_a = 0; try A51 a; catch(Error e) {}         assert(A51_a == 1);
+  A51_a = 0; if (0) switch(1) { A51 a; default: } assert(A51_a == 0);
+  A51_a = 0; if (0) switch(1) { default: A51 a; } assert(A51_a == 0);
+  A51_a = 0; final switch(0) A51 a;               assert(A51_a == 0);
+  A51_a = 0; A51 a; with(a) A51 b;                assert(A51_a == 1); // should be 2, right?
 }

Later,
Brad

On 6/2/11 11:04 AM, Brad Roberts wrote:
> So, is d (and thus dmd) supposed to deal with that case better?  I assume no since there's no case 1:, and that the test
> should be changed to:
>    if (0) switch(1) { A51 a; default: }

In this case a should never be destroyed even on if (1) as no control flow reaches it.

(On another vein, I'm surprised that that ever compiled. In C and C++ I recall it's illegal to have a label followed by no code.)

> Of course, the opposite is also interesting in terms of 'in what cases is a actually constructed/destructed':
>    if (0) switch(1) { default: A51 a; }

Diff looks good to me, and the last line should indeed call two destructors.


Andrei

On 6/2/2011 9:22 AM, Andrei Alexandrescu wrote:
> On 6/2/11 11:04 AM, Brad Roberts wrote:
>> So, is d (and thus dmd) supposed to deal with that case better?  I assume no since there's no case 1:, and that the test
>> should be changed to:
>>    if (0) switch(1) { A51 a; default: }
> 
> In this case a should never be destroyed even on if (1) as no control flow reaches it.
> 
> (On another vein, I'm surprised that that ever compiled. In C and C++ I recall it's illegal to have a label followed by no code.)
> 
>> Of course, the opposite is also interesting in terms of 'in what cases is a actually constructed/destructed':
>>    if (0) switch(1) { default: A51 a; }
> 
> Diff looks good to me, and the last line should indeed call two destructors.
> 
> 
> Andrei

I just double checked, neither gcc or g++ accept an empty default statement.  Obviously dmd does.

Adding two more tests:

   A51_a = 0; if (1) switch(1) { A51 a; default: } assert(A51_a == 1); // should be 0, right?
   A51_a = 0; if (1) switch(1) { default: A51 a; } assert(A51_a == 1);

So.. two bugs.. oh Walter..

On 6/2/2011 9:04 AM, Brad Roberts wrote:
> With the changes to switch to require a default handler, there's some fall-out in runnable/sdtor.d.
>
> line 1271:
>    if (0) switch(1) A51 a;
>
> runnable/sdtor.d(1271): Error: non-final switch statement must have a default

I just made it a final switch.

> +  A51_a = 0; A51 a; with(a) A51 b;                assert(A51_a == 1); // should be 2, right?
>

No, 1. Because 'a' isn't destructed until the end of the scope, which hasn't happened by the the time the assert is called.

On Jun 2, 2011, at 11:05 AM, Walter Bright <walter at digitalmars.com> wrote:

> 
> 
> On 6/2/2011 9:04 AM, Brad Roberts wrote:
>> With the changes to switch to require a default handler, there's some fall-out in runnable/sdtor.d.
>> 
>> line 1271:
>>   if (0) switch(1) A51 a;
>> 
>> runnable/sdtor.d(1271): Error: non-final switch statement must have a default
> 
> I just made it a final switch

Final shouldn't change the error.  The case of 1 isn't handled.

>> +  A51_a = 0; A51 a; with(a) A51 b;                assert(A51_a == 1); // should be 2, right?
>> 
> 
> No, 1. Because 'a' isn't destructed until the end of the scope, which hasn't happened by the the time the assert is called.

Good catch.  Each of those should probably be pushed into an enclosing block.

On 6/2/2011 11:15 AM, Brad Roberts wrote:
> 
> 
> On Jun 2, 2011, at 11:05 AM, Walter Bright <walter at digitalmars.com> wrote:
> 
>>
>>
>> On 6/2/2011 9:04 AM, Brad Roberts wrote:
>>> With the changes to switch to require a default handler, there's some fall-out in runnable/sdtor.d.
>>>
>>> line 1271:
>>>   if (0) switch(1) A51 a;
>>>
>>> runnable/sdtor.d(1271): Error: non-final switch statement must have a default
>>
>> I just made it a final switch
> 
> Final shouldn't change the error.  The case of 1 isn't handled.
> 
>>> +  A51_a = 0; A51 a; with(a) A51 b;                assert(A51_a == 1); // should be 2, right?
>>>
>>
>> No, 1. Because 'a' isn't destructed until the end of the scope, which hasn't happened by the the time the assert is called.
> 
> Good catch.  Each of those should probably be pushed into an enclosing block.

The new contents of test51:

+  A51_a = 0; { while(0) A51 a;                      } assert(A51_a == 0);
+  A51_a = 0; { if(0) A51 a;                         } assert(A51_a == 0);
+  A51_a = 0; { if(1){} else A51 a;                  } assert(A51_a == 0);
+  A51_a = 0; { for(;0;) A51 a;                      } assert(A51_a == 0);
+  A51_a = 0; { if (1) { A51 a; }                    } assert(A51_a == 1);
+  A51_a = 0; { if (1) A51 a;                        } assert(A51_a == 1);
+  A51_a = 0; { if(0) {} else A51 a;                 } assert(A51_a == 1);
+  A51_a = 0; { if (0) for(A51 a;;) {}               } assert(A51_a == 0);
+  A51_a = 0; { if (0) for(;;) A51 a;                } assert(A51_a == 0);
+  A51_a = 0; { do A51 a; while(0);                  } assert(A51_a == 1);
+  A51_a = 0; { if (0) while(1) A51 a;               } assert(A51_a == 0);
+  A51_a = 0; { try A51 a; catch(Error e) {}         } assert(A51_a == 1);
+  A51_a = 0; { if (0) final switch(1) A51 a;        } assert(A51_a == 0); // should fail to build
+  A51_a = 0; { if (0) switch(1) { A51 a; default: } } assert(A51_a == 0);
+  A51_a = 0; { if (0) switch(1) { default: A51 a; } } assert(A51_a == 0);
+  A51_a = 0; { if (1) switch(1) { A51 a; default: } } assert(A51_a == 1); // should be 0, right?
+  A51_a = 0; { if (1) switch(1) { default: A51 a; } } assert(A51_a == 1);
+  A51_a = 0; { final switch(0) A51 a;               } assert(A51_a == 0);
+  A51_a = 0; { A51 a; with(a) A51 b;                } assert(A51_a == 2);

Any reason not to commit this followed by opening two bugs for the two commented lines?

no.

On 6/2/2011 10:16 PM, Brad Roberts wrote:
>
> Any reason not to commit this followed by opening two bugs for the two commented lines?
>