View mode: basic / threaded / horizontal-split · Log in · Help
December 09, 2011
Re: Comma operator = broken design
On Fri, 09 Dec 2011 11:39:55 -0000, Timon Gehr <timon.gehr@gmx.ch> wrote:
> On 12/09/2011 10:19 AM, Don wrote:
>> On 08.12.2011 20:22, Timon Gehr wrote:
>>> Phobos would break, for example. And some of my code too.
>>
>> Are there any cases where you're using comma outside of for loops?
>
> Yes, one for every 65 LOC.
>
>> I wonder how much would break if were made illegal everywhere else.
>
> These are the occurences of the comma operator in directory 'std':
>
> return r2.empty ? (r1 = r, true) : false;
> return binaryFun!pred(r.front, e) ? (r.popFront(), true) : false;
>
> if (f.flPlus)
>      signChar = '+', ++minw;
> else if (f.flSpace)
>      signChar = ' ', ++minw;
>
> if (std.ascii.toLower(p.front) == 'n' &&
>         (p.popFront(), std.ascii.toLower(p.front) == 'f') &&
>         (p.popFront(), p.empty))
>
> 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"));
>
> if (indexStart != 0)
>      formatValue(w, indexStart, f), put(w, '$');
>
> if (c == '\"' || c == '\\')
>      put(w, '\\'), put(w, c);
> else
>      put(w, c);
>
> return (++mi.m_cRefs, cast(HXModule)mi);
> return (++mi.m_cRefs, hModule);
>
> return
>      c <= 0x7F ? 1
>      : c <= 0x7FF ? 2
>      : c <= 0xFFFF ? 3
>      : c <= 0x10FFFF ? 4
>      : (assert(false), 6);

All of the above could be improved with the removal of the comma operator,  
IMO.  They are needlessly complicated/confusing with it.

R

-- 
Using Opera's revolutionary email client: http://www.opera.com/mail/
December 09, 2011
Re: Comma operator = broken design
Timon Gehr:

> These are the occurences of the comma operator in directory 'std':
> 
> return r2.empty ? (r1 = r, true) : false;
> return binaryFun!pred(r.front, e) ? (r.popFront(), true) : false;
> 
> if (f.flPlus)
>      signChar = '+', ++minw;
> else if (f.flSpace)
>      signChar = ' ', ++minw;
> 
> if (std.ascii.toLower(p.front) == 'n' &&
>         (p.popFront(), std.ascii.toLower(p.front) == 'f') &&
>         (p.popFront(), p.empty))
> 
> 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"));
> 
> if (indexStart != 0)
>      formatValue(w, indexStart, f), put(w, '$');
> 
> if (c == '\"' || c == '\\')
>      put(w, '\\'), put(w, c);
> else
>      put(w, c);
> 
> return (++mi.m_cRefs, cast(HXModule)mi);
> return (++mi.m_cRefs, hModule);
> 
> return
>      c <= 0x7F ? 1
>      : c <= 0x7FF ? 2
>      : c <= 0xFFFF ? 3
>      : c <= 0x10FFFF ? 4
>      : (assert(false), 6);

It's ugly code worth fixing/rewriting. If I see something like that in production code, I always burn it with fire and rewrite it.
Turning such usages of comma operator into syntax errors looks like a general improvement for D.

Bye,
bearophile
December 09, 2011
Re: Comma operator = broken design
On Fri, 09 Dec 2011 12:00:57 -0000, Timon Gehr <timon.gehr@gmx.ch> wrote:
> On 12/09/2011 10:26 AM, Jonathan M Davis wrote:
>> On Friday, December 09, 2011 10:19:18 Don wrote:
>>> Are there any cases where you're using comma outside of for loops?
>>> I wonder how much would break if were made illegal everywhere else.
>>
>> I'm sure that it would break code, but most people consider it bad  
>> practice to
>> use the comma operator for much outside of for loops.
>
> 'most people'?

Yes, I would guess that you're in the minority here.

>> Occasionally, it's
>> useful to use one in an expression, but on the whole, it's just  
>> confusing and
>> error-prone.
>
> It is confusing to people who don't know the language. It is a simple  
> construct. In my experience, it is certainly not error prone. If you are  
> aware that such an operator exists.

It's error prone in cases where you use it without realising, as in the  
example I posted.

>> The resulting code may not be as compact, but it
>> wouldn't be hard to write.
>
> It would be a PITA in some cases.

Really?  Give us an example where not having comma makes things  
significantly difficult.

>> And unless you're dealing with a programmer who
>> uses it uncommonly often, not much code is going to break.
>
> I _am_ such a programmer.

So it seems :)  I don't want to make your life harder but I think this  
change would make life easier for a large number of people, a small amount  
of the time.  The equation is unbalanced in favour of it's removal, IMO.

-- 
Using Opera's revolutionary email client: http://www.opera.com/mail/
December 09, 2011
Re: Comma operator = broken design
On Fri, 09 Dec 2011 11:39:55 -0000, Timon Gehr <timon.gehr@gmx.ch> wrote:
> These are the occurences of the comma operator in directory 'std':

Here is my /very/ quick re-write of each without looking at the context of  
each snippet.  There may be much better re-writes in context.  Re-writing  
this was problematic /because/ the comma operator makes things that much  
more confusing to me.  Especially example #5 below where the last ,  
actually separates enforce parameters!

1)

> return r2.empty ? (r1 = r, true) : false;

if (!r2.empty) return false;
r1 = r;
return true;


2)

> return binaryFun!pred(r.front, e) ? (r.popFront(), true) : false;

if (!binaryFun!pred(r.front, e)) return false;	
r.popFront();
return true;


3)

> if (f.flPlus)
>      signChar = '+', ++minw;
> else if (f.flSpace)
>      signChar = ' ', ++minw;

This is purely {}; avoidance it seems..

if (f.flPlus)
{
  signChar = '+';
  ++minw;
}
else if (f.flSpace)
{
  signChar = ' ';
  ++minw;
}


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


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


6)

> if (indexStart != 0)
>      formatValue(w, indexStart, f), put(w, '$');

More {}; avoidance..

if (indexStart != 0)
{
    formatValue(w, indexStart, f);
	put(w, '$');
}


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


8)

> return (++mi.m_cRefs, cast(HXModule)mi);

less (), one more ; and <enter>..

++mi.m_cRefs;
return cast(HXModule)mi;


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;

None of the above look significantly harder without the comma operator and  
quite a few are far clearer (to me) without it.

R

-- 
Using Opera's revolutionary email client: http://www.opera.com/mail/
December 09, 2011
Re: Comma operator = broken design
On Fri, 09 Dec 2011 22:39:55 +1100, Timon Gehr <timon.gehr@gmx.ch> wrote:

> These are the occurences of the comma operator in directory 'std': ...

OMG! What ever happened to the idea that source code is meant to AID  
reading programs and not making it obscured.

These are examples for a 'shame' file, or "how not to write useful code".


-- 
Derek Parnell
Melbourne, Australia
December 09, 2011
Re: Comma operator = broken design
On 12/09/2011 01:11 PM, Regan Heath wrote:
> On Fri, 09 Dec 2011 12:00:57 -0000, Timon Gehr <timon.gehr@gmx.ch> wrote:
>> On 12/09/2011 10:26 AM, Jonathan M Davis wrote:
>>> On Friday, December 09, 2011 10:19:18 Don wrote:
>>>> Are there any cases where you're using comma outside of for loops?
>>>> I wonder how much would break if were made illegal everywhere else.
>>>
>>> I'm sure that it would break code, but most people consider it bad
>>> practice to
>>> use the comma operator for much outside of for loops.
>>
>> 'most people'?
>
> Yes, I would guess that you're in the minority here.
>

'most people' do not care about this discussion, so lets stop it. :o)

What I wanted to say is just that throwing in 'most people' does not 
help an argument, because a) there is no evidence and b) 'most people' 
are in general just as often (or even more) wrong than 'a few people'.

>>> Occasionally, it's
>>> useful to use one in an expression, but on the whole, it's just
>>> confusing and
>>> error-prone.
>>
>> It is confusing to people who don't know the language. It is a simple
>> construct. In my experience, it is certainly not error prone. If you
>> are aware that such an operator exists.
>
> It's error prone in cases where you use it without realising, as in the
> example I posted.
>

That is true for every language construct.

>>> The resulting code may not be as compact, but it
>>> wouldn't be hard to write.
>>
>> It would be a PITA in some cases.
>
> Really? Give us an example where not having comma makes things
> significantly difficult.

I already agreed that rewriting the code is trivial. It is just that I 
usually do not want to spend lots of my time on trivial stuff.

>
>>> And unless you're dealing with a programmer who
>>> uses it uncommonly often, not much code is going to break.
>>
>> I _am_ such a programmer.
>
> So it seems :)

If you carefully read my posts you will find that I did never say that I 
am against removing the comma operator.

I would like comma to have tuple semantics. Comma should keep
its precedence level and then work like this:

if(foo) bar, baz, qux; // the value of the tuple is unused, so no change 
in semantics

if(bar()||(foo(), *++p==(*++x)++)[1]){} // the value is used, so an 
explicit index is necessary

It would make the language more consistent because the comma in 
expressions would be the same comma as the one that is used elsewhere.

That is against the C compatibility rule though, and it would in some 
cases silently break code.

> I don't want to make your life harder but I think this
> change would make life easier for a large number of people, a small
> amount of the time. The equation is unbalanced in favour of it's
> removal, IMO.
>

This part of your post is somewhat contradictory.
December 09, 2011
Re: Comma operator = broken design
Regan Heath wrote:
> On Fri, 09 Dec 2011 12:00:57 -0000, Timon Gehr <timon.gehr@gmx.ch> wrote:
>> On 12/09/2011 10:26 AM, Jonathan M Davis wrote:
>>> And unless you're dealing with a programmer who
>>> uses it uncommonly often, not much code is going to break.
>>
>> I _am_ such a programmer.
>
> So it seems :) I don't want to make your life harder but I think this
> change would make life easier for a large number of people [...]

What about just not using comma operator? Yes, I know about keyboard 
issue. There are many cases when 'typoed' code compiles but doesn't work 
as programmer would expect. This is not limited to comma op.

Some programmers do not like gotos, comma operator or other language 
constructs. Some like the opposites. I would suggest making a patch to 
the compiler to introduce a new pragma:

pragma(disable, commaOp);

and programmers will be able to place it in every source file they want to.
December 09, 2011
Re: Comma operator = broken design
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.
December 09, 2011
Re: Comma operator = broken design
On 12/09/2011 01:56 PM, Derek wrote:
> On Fri, 09 Dec 2011 22:39:55 +1100, Timon Gehr <timon.gehr@gmx.ch> wrote:
>
>> These are the occurences of the comma operator in directory 'std': ...
>
> OMG! What ever happened to the idea that source code is meant to AID
> reading programs and not making it obscured.
>
> These are examples for a 'shame' file, or "how not to write useful code".
>
>

You are exaggerating.

http://www.ioccc.org/
December 09, 2011
Re: Comma operator = broken design
On Fri, 09 Dec 2011 13:15:47 -0000, Timon Gehr <timon.gehr@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/
1 2 3 4 5 6 7
Top | Discussion index | About this forum | D home