Comma operator = broken design

[Regan Heath]

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.

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

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

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

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

:)

-- 
Using Opera's revolutionary email client: http://www.opera.com/mail/