Comma operator = broken design

[Timon Gehr]

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.

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

>
> 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);


>
> 8)
>
>> return (++mi.m_cRefs, cast(HXModule)mi);
>
> less (), one more ; and <enter>..
>
> ++mi.m_cRefs;
> return cast(HXModule)mi;
>

() would not be necessary.

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


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