Sign in to follow this  
GGLucas

[PATCH] floating point comparisons involving infinity

Recommended Posts

Hello! I work with Andrew (Thy Reaper), and have reported a couple of bugs through him before. This one seemed easy enough to fix so I decided to submit a patch instead.

I bound float-infinity as a constant global to the scripts, and found out that because of the way AS does its comparisons, INFINITY != INFINITY, which is counter to the behaviour of infinities in C/C++. If that's intended feel free to ignore the patch, but I can't think of a reason why it would be.

To me, at least, it looks like the way the comparisons are done is a premature optimization. The compiler can figure out how to optimize it better, and from what I can tell from [url="http://pastie.org/private/qnrgenrbaqmzzlmocwlzha"]a trivial decompile[/url], it might very well do better on the plain version that I changed it to (two compares versus a subtract and two compares). That is, of course, all highly machine- and compiler-specific speculation, but in general I think it's better to leave it to the C++ compiler to decide what's best.

[source lang="cpp"]Index: sdk/angelscript/source/as_context.cpp
===================================================================
--- sdk/angelscript/source/as_context.cpp (revision 1413)
+++ sdk/angelscript/source/as_context.cpp (working copy)
@@ -2149,10 +2149,11 @@
// Comparisons
case asBC_CMPd:
{
- double dbl = *(double*)(l_fp - asBC_SWORDARG0(l_bc)) - *(double*)(l_fp - asBC_SWORDARG1(l_bc));
- if( dbl == 0 ) *(int*)&m_regs.valueRegister = 0;
- else if( dbl < 0 ) *(int*)&m_regs.valueRegister = -1;
- else *(int*)&m_regs.valueRegister = 1;
+ double dbl1 = *(double*)(l_fp - asBC_SWORDARG0(l_bc));
+ double dbl2 = *(double*)(l_fp - asBC_SWORDARG1(l_bc));
+ if( dbl1 == dbl2 ) *(int*)&m_regs.valueRegister = 0;
+ else if( dbl1 < dbl2 ) *(int*)&m_regs.valueRegister = -1;
+ else *(int*)&m_regs.valueRegister = 1;
l_bc += 2;
}
break;
@@ -2170,20 +2171,22 @@

case asBC_CMPf:
{
- float f = *(float*)(l_fp - asBC_SWORDARG0(l_bc)) - *(float*)(l_fp - asBC_SWORDARG1(l_bc));
- if( f == 0 ) *(int*)&m_regs.valueRegister = 0;
- else if( f < 0 ) *(int*)&m_regs.valueRegister = -1;
- else *(int*)&m_regs.valueRegister = 1;
+ float f1 = *(float*)(l_fp - asBC_SWORDARG0(l_bc));
+ float f2 = *(float*)(l_fp - asBC_SWORDARG1(l_bc));
+ if( f1 == f2 ) *(int*)&m_regs.valueRegister = 0;
+ else if( f1 < f2 ) *(int*)&m_regs.valueRegister = -1;
+ else *(int*)&m_regs.valueRegister = 1;
l_bc += 2;
}
break;

case asBC_CMPi:
{
- int i = *(int*)(l_fp - asBC_SWORDARG0(l_bc)) - *(int*)(l_fp - asBC_SWORDARG1(l_bc));
- if( i == 0 ) *(int*)&m_regs.valueRegister = 0;
- else if( i < 0 ) *(int*)&m_regs.valueRegister = -1;
- else *(int*)&m_regs.valueRegister = 1;
+ int i1 = *(int*)(l_fp - asBC_SWORDARG0(l_bc));
+ int i2 = *(int*)(l_fp - asBC_SWORDARG1(l_bc));
+ if( i1 == i2 ) *(int*)&m_regs.valueRegister = 0;
+ else if( i1 < i2 ) *(int*)&m_regs.valueRegister = -1;
+ else *(int*)&m_regs.valueRegister = 1;
l_bc += 2;
}
break;
@@ -2192,20 +2195,22 @@
// Comparisons with constant value
case asBC_CMPIi:
{
- int i = *(int*)(l_fp - asBC_SWORDARG0(l_bc)) - asBC_INTARG(l_bc);
- if( i == 0 ) *(int*)&m_regs.valueRegister = 0;
- else if( i < 0 ) *(int*)&m_regs.valueRegister = -1;
- else *(int*)&m_regs.valueRegister = 1;
+ int i1 = *(int*)(l_fp - asBC_SWORDARG0(l_bc));
+ int i2 = asBC_INTARG(l_bc);
+ if( i1 == i2 ) *(int*)&m_regs.valueRegister = 0;
+ else if( i1 < i2 ) *(int*)&m_regs.valueRegister = -1;
+ else *(int*)&m_regs.valueRegister = 1;
l_bc += 2;
}
break;

case asBC_CMPIf:
{
- float f = *(float*)(l_fp - asBC_SWORDARG0(l_bc)) - asBC_FLOATARG(l_bc);
- if( f == 0 ) *(int*)&m_regs.valueRegister = 0;
- else if( f < 0 ) *(int*)&m_regs.valueRegister = -1;
- else *(int*)&m_regs.valueRegister = 1;
+ float f1 = *(float*)(l_fp - asBC_SWORDARG0(l_bc));
+ float f2 = asBC_FLOATARG(l_bc);
+ if( f1 == f2 ) *(int*)&m_regs.valueRegister = 0;
+ else if( f1 < f2 ) *(int*)&m_regs.valueRegister = -1;
+ else *(int*)&m_regs.valueRegister = 1;
l_bc += 2;
}
break;
[/source]


PS. If you feel like setting one up, github is actually quite amazing for open source projects - it makes contributing a much smoother process on both ends. As a contributor I could clone the official repository and use version control myself instead of having to submit loose patches, and as a developer all you need to do is look at the pull request, inspect the patches and press the nice accept button instead of having to apply a bunch of separate patches and create commits. Just a suggestion.

Share this post


Link to post
Share on other sites
This was definitely not intentional from my part, nor was it an attempt to optimize the code.

Thanks for the patch. I'll have it applied in the next check-in.


Maybe I'll look into using github in the future, others have made the same suggestion before. It's just that I'm comfortable with how things are now and don't really feel a need to change it myself. The act of manually merging the patches I receive gives me a better chance to actually analyse what is being modified, to think about the impacts it will have.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this