Comma operator = broken design
Regan Heath
regan at netmail.co.nz
Fri Dec 9 06:25:34 PST 2011
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/
More information about the Digitalmars-d
mailing list