Comma operator = broken design

[Steven Schveighoffer]

On Fri, 09 Dec 2011 09:25:34 -0500, Regan Heath <regan at netmail.co.nz>  
wrote:

> 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.
>

goto in this case is acceptable.  It's a goto case statement, which  
technically should be required for fall-through.

>>>>> 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.

This case is a perfect example of what is right and wrong with the comma  
operator.

With the comma operator, the single statement avoids code duplication,  
which is good for maintenance.  However, it's extremely hard to see what's  
going on.  Timon, please accept that you may be one of the few who reads  
that statement and sees perfectly what's happening, I needed to read  
Regan's version to understand what it does without hurting my head too  
much.

My opinion?  I think it's better written like this:

p.popFront();
bool bad = void;
if(!(bad = p.empty || std.ascii.toUpper(p.front) != 'A'))
{
    p.popFront();
    bad = p.empty || std.ascii.toUpper(p.front) != 'N';
}
if(bad)
    throw new ConvException("error converting input to floating point");

And I'd probably reread it again, and throw in some comments to help me  
remember what the f*** I was doing :)

I don't see a smaller solution, or one that doesn't use a temporary,  
without using the comma operator or duplicating the throw/enforce call.

-Steve