View mode: basic / threaded / horizontal-split · Log in · Help
December 09, 2011
Re: Comma operator = broken design
On Fri, 09 Dec 2011 13:01:12 -0000, Timon Gehr <timon.gehr@gmx.ch> wrote:
> 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)

Sure.

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

Excellent.

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

How so?

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 13:15:22 -0000, Piotr Szturmaj <bncrbme@jadamspam.pl>  
wrote:

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

True, but D has already turned a good number of those into errors.  This  
is another example where it could for very little cost, IMO.

R

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

> 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 may not be error prone to write, but it is error prone to read, because
the comma-operator tends to hide in the language noise produced by all those
brackets, semicolons, commas, parenthesis and curly braces.

I don't like it, if I have to be uber careful when reading trivial code.
December 09, 2011
Re: Comma operator = broken design
On 12/09/2011 02:28 PM, Regan Heath wrote:
> 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.

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.
>
> :)
>
December 09, 2011
Re: Comma operator = broken design
On 12/09/2011 02:33 PM, Regan Heath wrote:
> On Fri, 09 Dec 2011 13:01:12 -0000, Timon Gehr <timon.gehr@gmx.ch> wrote:
>> 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)
>
> Sure.
>
>>> 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.
>
> Excellent.
>
>>> 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.
>
> How so?
>
> R
>

Removing the comma operator makes my life harder. You don't want to do 
so. You want to remove the comma operator. Conclusion: Today is Monday.
December 09, 2011
Re: Comma operator = broken design
On 12/09/2011 02:38 PM, Tobias Pankrath wrote:
> Timon Gehr wrote:
>
>> 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 may not be error prone to write, but it is error prone to read, because
> the comma-operator tends to hide in the language noise produced by all those
> brackets, semicolons, commas, parenthesis and curly braces.
>
> I don't like it, if I have to be uber careful when reading trivial code.

You always have to be careful when reading code.
I prefer when there is not a lot to read (dense code).
December 09, 2011
Re: Comma operator = broken design
On Fri, 09 Dec 2011 13:47:14 -0000, Timon Gehr <timon.gehr@gmx.ch> wrote:
>>>> 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.
>>
>> How so?
>>
>> R
>>
>
> Removing the comma operator makes my life harder. You don't want to do  
> so. You want to remove the comma operator. Conclusion: Today is Monday.

:p

1. I don't want to make your life harder, but (despite that)
2. I do want to remove the comma operator

Nothing contradictory there at all, simply cost/benefit.

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 13:42:34 -0000, Timon Gehr <timon.gehr@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@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/
December 09, 2011
Re: Comma operator = broken design
On 12/09/2011 03:25 PM, Regan Heath wrote:
> On Fri, 09 Dec 2011 13:42:34 -0000, Timon Gehr <timon.gehr@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@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
>

What you might be missing is that assert(false) is not compiled out in 
release mode. It emits a 'hlt' instruction which kills your program. 
However, your assert(c <= 0x10FFFF); will be removed in release mode.
December 09, 2011
Re: Comma operator = broken design
I didn't notice the other parts of your post before.

On 12/09/2011 03:25 PM, Regan Heath wrote:
> On Fri, 09 Dec 2011 13:42:34 -0000, Timon Gehr <timon.gehr@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@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.
>

The code was probably written that way to give an optimal implementation 
that does not use goto. How would you refactor the 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.
>

I don't like the nested if solution. Also it does not work that well in 
the general case because you might have a || operator somewhere instead 
of just && operators.
1 2 3 4 5 6 7
Top | Discussion index | About this forum | D home