December 09, 2011
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
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
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
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
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
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
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
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
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
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.