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