A little tiny horror
-
Gary Wheeler wrote:
I replaced it with the following expression:
And in the process introduced a subtle bug ;P
xacc.ide - now with IronScheme support
IronScheme - 1.0 alpha 1 out nowGood catch! I assume you are referring to the different results when having i with values smaller than 0 and bigger than 31... Robert
-
I found this expression in our current source code:
int i;
//...
(int)pow(2,i)This was in code written by a senior developer :wtf:. I replaced it with the following expression:
(1 << i)
Software Zen:
delete this; -
David Kentley wrote:
It's possible that, as an optimization, "fixing" this has zero impact on real world performance while (slightly) obfuscating the code.
I'm pretty sure pow() only takes and returns doubles which means the original code is casting two ints in and one back out in addition to its internal pow goodness. I'm guessing the shift is a wee bit faster. :)
Robert Surtees wrote:
I'm guessing the shift is a wee bit faster.
Which, as was argued here, is very probably of no importance, while the resulting obfuscation of the intent of the calculation is important.
Let's think the unthinkable, let's do the undoable, let's prepare to grapple with the ineffable itself, and see if we may not eff it after all.
Douglas Adams, "Dirk Gently's Holistic Detective Agency" -
I'd have to see more of the code to decide if this really deserves to be a horror. It's possible that, as an optimization, "fixing" this has zero impact on real world performance while (slightly) obfuscating the code. I'm a much bigger fan of readable code than optimizations with negligible performance improvements. (Not that "2 << i" is that unreadable, but you get the idea.)
Faith is a fine invention For gentlemen who see; But microscopes are prudent In an emergency! -Emily Dickinson
The original surrounding code looked something like this:
for (int i = 0; i < 16; i++) {
CString name;
name.Format(_T("Stuff_%d.dat"),(int)pow(2,i));
};Thinking about it, if I wanted to maximize readability, I would have done this:
int name_value = 1;
for (int i = 0; i < 16; i++) {
CString name;
name.Format(_T("Stuff_%d.dat"),name_value);
name_value *= 2;
};The only reason I found this is a compiler error in the original
(int)pow(2,i)expression from VS2008 (ambiguous override).Software Zen:
delete this; -
The code got optimized for readability for the 'junior' programmers. And it has the potential to get easily chanced to (int)pow(3,i). I say this to defend your senior... :-O
Greetings from Germany
KarstenK wrote:
defend your senior...
Actually, I'm senior to the guy that wrote this. I just thought that the guy should have picked a better way to achieve the desired affect.
Software Zen:
delete this; -
Gary Wheeler wrote:
I replaced it with the following expression:
And in the process introduced a subtle bug ;P
xacc.ide - now with IronScheme support
IronScheme - 1.0 alpha 1 out nowActually not; see my reply above.
Software Zen:
delete this; -
Good catch! I assume you are referring to the different results when having i with values smaller than 0 and bigger than 31... Robert
Just smaller than 0, but as he is casting to int 31 or above could be a problem too, not sure if C (which I assume this is) will create a 'long' or 'long long' or whatever they use from the shift. But the result of usage of negative numbers are clear undefined.
xacc.ide - now with IronScheme support
IronScheme - 1.0 alpha 1 out now -
KarstenK wrote:
defend your senior...
Actually, I'm senior to the guy that wrote this. I just thought that the guy should have picked a better way to achieve the desired affect.
Software Zen:
delete this;Like:
int pow(int i, int j)
{
switch (j)
case 0: return 1;
case 1: return i;
case 2: return i * i;
case 3: return i * i * i;
case 4: return i * i * i * i;
...
};P
xacc.ide - now with IronScheme support
IronScheme - 1.0 alpha 1 out now -
Like:
int pow(int i, int j)
{
switch (j)
case 0: return 1;
case 1: return i;
case 2: return i * i;
case 3: return i * i * i;
case 4: return i * i * i * i;
...
};P
xacc.ide - now with IronScheme support
IronScheme - 1.0 alpha 1 out nowNope. You forgot
break;statement. Oh pardon, you're senior! :-DIf the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler. -- Alfonso the Wise, 13th Century King of Castile.
[my articles] -
I found this expression in our current source code:
int i;
//...
(int)pow(2,i)This was in code written by a senior developer :wtf:. I replaced it with the following expression:
(1 << i)
Software Zen:
delete this;Gary Wheeler wrote:
This was in code written by a senior developer
When seniority approaches retirement... :-D
If the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler. -- Alfonso the Wise, 13th Century King of Castile.
[my articles] -
Nope. You forgot
break;statement. Oh pardon, you're senior! :-DIf the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler. -- Alfonso the Wise, 13th Century King of Castile.
[my articles] -
I'd have to see more of the code to decide if this really deserves to be a horror. It's possible that, as an optimization, "fixing" this has zero impact on real world performance while (slightly) obfuscating the code. I'm a much bigger fan of readable code than optimizations with negligible performance improvements. (Not that "2 << i" is that unreadable, but you get the idea.)
Faith is a fine invention For gentlemen who see; But microscopes are prudent In an emergency! -Emily Dickinson
David Kentley wrote:
Not that "2 << i" is that unreadable, but you get the idea.
In fact it isn't unreadable, it is wrong. :-D
If the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler. -- Alfonso the Wise, 13th Century King of Castile.
[my articles] -
Robert Surtees wrote:
I'm guessing the shift is a wee bit faster.
Which, as was argued here, is very probably of no importance, while the resulting obfuscation of the intent of the calculation is important.
Let's think the unthinkable, let's do the undoable, let's prepare to grapple with the ineffable itself, and see if we may not eff it after all.
Douglas Adams, "Dirk Gently's Holistic Detective Agency"jhwurmbach wrote:
Robert Surtees wrote: I'm guessing the shift is a wee bit faster. Which, as was argued here, is very probably of no importance, while the resulting obfuscation of the intent of the calculation is important.
I'd say it's important. Gary's XT doesn't have an 8087 plugged in.
-
David Kentley wrote:
It's possible that, as an optimization, "fixing" this has zero impact on real world performance while (slightly) obfuscating the code.
I'm pretty sure pow() only takes and returns doubles which means the original code is casting two ints in and one back out in addition to its internal pow goodness. I'm guessing the shift is a wee bit faster. :)
Robert Surtees wrote:
I'm pretty sure pow() only takes and returns doubles which means the original code is casting two ints in and one back out in addition to its internal pow goodness. I'm guessing the shift is a wee bit faster.
VC++ compiler is smart enough to implement it using shift instead of
pow. Anyway, IMHO shift syntax is far more clean. :)If the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler. -- Alfonso the Wise, 13th Century King of Castile.
[my articles] -
Like:
int pow(int i, int j)
{
switch (j)
case 0: return 1;
case 1: return i;
case 2: return i * i;
case 3: return i * i * i;
case 4: return i * i * i * i;
...
};P
xacc.ide - now with IronScheme support
IronScheme - 1.0 alpha 1 out nowint pow(int i, int j) { switch (j) { default: return 0; case 0: return 1; case 32: i *= i; case 31: i *= i; case 30: i *= i; ... case 1: return i; } }.... ewwwwwTim Smith I'm going to patent thought. I have yet to see any prior art.
-
int pow(int i, int j) { switch (j) { default: return 0; case 0: return 1; case 32: i *= i; case 31: i *= i; case 30: i *= i; ... case 1: return i; } }.... ewwwwwTim Smith I'm going to patent thought. I have yet to see any prior art.
-
Shift left is way cooler than pow any day. And in the end it's all about how good the code looks, eh? ;)
cheers, Chris Maunder
CodeProject.com : C++ MVP
words to live code by! :D
We are a big screwed up dysfunctional psychotic happy family - some more screwed up, others more happy, but everybody's psychotic joint venture definition of CP
My first real C# project | Linkify!| FoldWithUs! | sighist -
I'd have to see more of the code to decide if this really deserves to be a horror. It's possible that, as an optimization, "fixing" this has zero impact on real world performance while (slightly) obfuscating the code. I'm a much bigger fan of readable code than optimizations with negligible performance improvements. (Not that "2 << i" is that unreadable, but you get the idea.)
Faith is a fine invention For gentlemen who see; But microscopes are prudent In an emergency! -Emily Dickinson
Can you guarantee that floating point inaccuracies + truncation in the cast doesn't introduce a problem? A quick check shows they don't on VC8, but I wouldn#t have bet on it. Further, if your compiler uses the canonical (if simplistic) implementation of
double pow_simple(double x, double y) { return exp(y*log(x)); }you fail pretty quickly with pow(2,3) = 7.9999999999999982 To add a pitfall to a lurking bug: if you use the default %f specifier for that, it dutifully prints 8.000000, but truncates it to 7 when casting to int. Also, when using 64-bit integers, starting with pow(2,51) double loses on accuracy.We are a big screwed up dysfunctional psychotic happy family - some more screwed up, others more happy, but everybody's psychotic joint venture definition of CP
My first real C# project | Linkify!| FoldWithUs! | sighist -
Shift left is way cooler than pow any day. And in the end it's all about how good the code looks, eh? ;)
cheers, Chris Maunder
CodeProject.com : C++ MVP
-
int pow(int i, int j) { switch (j) { default: return 0; case 0: return 1; case 32: i *= i; case 31: i *= i; case 30: i *= i; ... case 1: return i; } }.... ewwwwwTim Smith I'm going to patent thought. I have yet to see any prior art.
(stack madness variant)
int imul(int i, int j)
{
if (j==0 ) return 0;
else return imul(i,j-1) + i;
}int ipow(int i, int j)
{
if (j==0) return 1;
else return imul(ipow(i,j-1) , i);
}:-D
If the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler. -- Alfonso the Wise, 13th Century King of Castile.
[my articles]
