Comma operator = broken design

[Timon Gehr]

I didn't notice the other parts of your post before.

On 12/09/2011 03:25 PM, Regan Heath 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.
>

The code was probably written that way to give an optimal implementation 
that does not use goto. How would you refactor the 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.
>

I don't like the nested if solution. Also it does not work that well in 
the general case because you might have a || operator somewhere instead 
of just && operators.