Comma operator = broken design

[Timon Gehr]

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

What you might be missing is that assert(false) is not compiled out in 
release mode. It emits a 'hlt' instruction which kills your program. 
However, your assert(c <= 0x10FFFF); will be removed in release mode.