2 replies to this topic

[GGLucas]

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 a trivial decompile, 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.

[Andreas Jonsson]

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.

[Andreas Jonsson]

I've checked in the patch to revision 1414.

Thanks,
Andreas