Comma operator = broken design

[Regan Heath]

On Fri, 09 Dec 2011 13:42:34 -0000, Timon Gehr <timon.gehr at gmx.ch> wrote:
> On 12/09/2011 02:28 PM, Regan Heath wrote:
>> On Fri, 09 Dec 2011 13:15:47 -0000, Timon Gehr <timon.gehr at gmx.ch>  
>> wrote:
>>> On 12/09/2011 01:36 PM, Regan Heath wrote:
>>>> 4)
>>>>
>>>>> if (std.ascii.toLower(p.front) == 'n' &&
>>>>> (p.popFront(), std.ascii.toLower(p.front) == 'f') &&
>>>>> (p.popFront(), p.empty))
>>>>
>>>> 'if' statements with side effects are yuck. I prefer the check for  
>>>> error
>>>> and bail style but you could use multiple layers of 'if' instead..
>>>>
>>>> if (std.ascii.toLower(p.front) != 'n') //error handling
>>>> p.popFront();
>>>> if (std.ascii.toLower(p.front) != 'f') //error handling
>>>> p.popFront();
>>>> if (!p.empty) //error handling
>>>>
>>>
>>> Your '//error handling' shortcut hides relevant information.
>>
>> What information? With context I could be more specific about what to do
>> for each/all.
>
> You can always grep the Phobos source to get context. Basically, you are  
> suggesting to replace the comma operator with gotos:
>
>      case 'i': case 'I':
>          p.popFront();
>          if (std.ascii.toLower(p.front) == 'n' &&
>                  (p.popFront(), std.ascii.toLower(p.front) == 'f') &&
> 	        (p.popFront(), p.empty))
>          {
>              // 'inf'
>              return sign ? -Target.infinity : Target.infinity;
>          }
>          goto default;
>      default: {}
>      }

If using 'goto' is the 'correct' agreed upon style for phobos then, yes.   
It's not my personal preference however and I'd probably refactor it  
further if it was my own code.

>>>> 5)
>>>>
>>>>> enforce((p.popFront(), !p.empty && std.ascii.toUpper(p.front) == 'A')
>>>>> && (p.popFront(), !p.empty && std.ascii.toUpper(p.front) == 'N'),
>>>>> new ConvException("error converting input to floating point"));
>>>>
>>>> This is blatant enforce abuse IMO..
>>>>
>>>> p.popFront();
>>>> if(p.empty || std.ascii.toUpper(p.front) != 'A'))
>>>> throw new ConvException("error converting input to floating point"));
>>>> p.popFront();
>>>> if(p.empty || std.ascii.toUpper(p.front) != 'N'))
>>>> throw new ConvException("error converting input to floating point"));
>>>>
>>>
>>> This is blatant code duplication.
>>
>> Of the throw line? Sure, and you can re-write with nested 'if' if you
>> prefer. I prefer the code duplication tho.
>>
>
> Code duplication is very error prone. Furthermore I never ever want to  
> duplicate _error handling_ code. That just clutters up the generated  
> machine code if the compiler does not manage to reverse engineer and  
> generate the proper solution.

I can't comment on the machine code aspect.  I don't find this particular  
duplication error prone, but if you do you can use the nested if that I've  
already mentioned.

>>>> 10)
>>>>
>>>>> return
>>>>> c <= 0x7F ? 1
>>>>> : c <= 0x7FF ? 2
>>>>> : c <= 0xFFFF ? 3
>>>>> : c <= 0x10FFFF ? 4
>>>>> : (assert(false), 6);
>>>>
>>>> *Much* clearer with the rewrite..
>>>>
>>>> assert(c <= 0x10FFFF);
>>>> return
>>>> c <= 0x7F ? 1
>>>> : c <= 0x7FF ? 2
>>>> : c <= 0xFFFF ? 3
>>>> : c <= 0x10FFFF ? 4
>>>> : 6;
>>>>
>>>
>>> This is a *much* better rewrite:
>>> assert(c <= 0x10FFFF);
>>> return
>>> c <= 0x7F ? 1
>>> : c <= 0x7FF ? 2
>>> : c <= 0xFFFF ? 3
>>> : 4;
>>
>> Again, I was missing context.. is the return value of 6 not required in
>> release mode?
>
> I was joking here. Your 'rewrite' of the original example changed its
> release mode semantics, therefore I did the same thing.

My version performs the assert in all cases, throws an assert error in the  
same/error cases, and still returns the correct/original values.  The only  
change is performing the assert in all cases, so I don't see how that is a  
problem.  Yours however is entirely broken (assuming the return value of 6  
is desired/required).

R

-- 
Using Opera's revolutionary email client: http://www.opera.com/mail/