Comma operator = broken design

[Timon Gehr]

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: {}
     }


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

>>> 7)
>>>
>>>> if (c == '\"' || c == '\\')
>>>> put(w, '\\'), put(w, c);
>>>> else
>>>> put(w, c);
>>>
>>> More {}; avoidance..
>>>
>>> if (c == '\"' || c == '\\')
>>> {
>>> put(w, '\\');
>>> put(w, c);
>>> }
>>> else
>>> {
>>> put(w, c);
>>> }
>>>
>>
>> No comma operator necessary to avoid {}:
>>
>> if(x == '"' || c == '\\') put(w, '\\');
>> put(w, c);
>
> Yeah, it was a /quick/ re-write without additional re-factoring (aside
> from removal of the comma).
>
>>> 8)
>>>
>>>> return (++mi.m_cRefs, cast(HXModule)mi);
>>>
>>> less (), one more ; and <enter>..
>>>
>>> ++mi.m_cRefs;
>>> return cast(HXModule)mi;
>>>
>>
>> () would not be necessary.
>
> True, but if you're going to use the comma operator, enclosing it in ()
> at makes it more obvious you've done so. I tend to use extra () when
> using the ?: operator for this reason.
>

You sort of made it look like the comma operator solution was more 
verbose. ;)

>>> 9)
>>>
>>>> return (++mi.m_cRefs, hModule);
>>>
>>> as above..
>>>
>>> ++mi.m_cRefs;
>>> return hModule;
>>>
>>>
>>> 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.


>
>>> None of the above look significantly harder without the comma operator
>>> and quite a few are far clearer (to me) without it.
>>>
>>
>> Yah, many of those occurences in Phobos are mostly unnecessary.
>
> :)
>