STYLE: Use std::abs, instead of doing if (x < 0) x = -x manually
#4943
Conversation
github-actions
bot
added
type:Testing
| RealType difference = static_cast<RealType>(t) - test.GetCenterPixel(); | ||
| if (NumericTraits<RealType>::IsNegative(difference)) | ||
| { | ||
| difference = -difference; | ||
| } | ||
| auto minimumDifference = static_cast<OutputPixelType>(difference); | ||
| RealType difference = std::abs(static_cast<RealType>(t) - test.GetCenterPixel()); | ||
| auto minimumDifference = static_cast<OutputPixelType>(difference); |
| // trying to avoid an expensive fabs | ||
| double tolerance = 1.e-05 * num; | ||
| if (tolerance < 0.0) | ||
| { | ||
| tolerance = -tolerance; | ||
| } | ||
| double tolerance = std::abs(1.e-05 * num); |
There was a problem hiding this comment.
The comment "trying to avoid an expensive fabs" appears to be introduced by commit cc69c19 "ENH: Added EvaluatePosition()...", Julien Jomier (@jjomier), Nov 16, 2004. fabs might have been expensive twenty years ago. But nowadays, it appears to be the other way around! When compiler optimization is enabled, fabs appears to be compiled to just a single machine instruction! While the manual approach (if (x < 0)...) appears to yield quite a few more machine instructions! Example at https://godbolt.org/z/M8e6x8ssh
double Call_fabs(double x)
{
return fabs(x);
}
double Call_std_abs(double x)
{
return std::abs(x);
}
double ManuallyEstimateAbs(double x)
{
if (x < 0)
{
return -x;
}
return x;
}Output:
Call_fabs(double):
andpd xmm0, XMMWORD PTR .LC0[rip]
ret
Call_std_abs(double):
andpd xmm0, XMMWORD PTR .LC0[rip]
ret
ManuallyEstimateAbs(double):
pxor xmm2, xmm2
movapd xmm3, xmm0
movapd xmm1, xmm0
xorpd xmm1, XMMWORD PTR .LC2[rip]
cmpltsd xmm0, xmm2
andpd xmm1, xmm0
andnpd xmm0, xmm3
orpd xmm0, xmm1
ret
.LC0:
.long -1
.long 2147483647
.long 0
.long 0
.LC2:
.long 0
.long -2147483648
.long 0
.long 0
Aims to make the code clearer. Cases found by regular expressions like ` (\w+) = -\1;` and ` \w+ \*= -1;` Removed old comments from inside two `DistanceToLine` member functions, saying that they are "trying to avoid an expensive fabs". `std::abs` isn't that expensive anymore nowadays.
N-Dekker
force-pushed
the
Use-std-abs-instead-of-manual-if
branch
from
ec2d28f
to
a1e7863
Compare
| @@ -329,11 +329,7 @@ FloatAlmostEqual(T x1, | |||
| return false; | |||
| } | |||
|
|
|||
| typename Detail::FloatIEEE<T>::IntType ulps = FloatDifferenceULP(x1, x2); | |||
There was a problem hiding this comment.
@N-Dekker Are you sure that this has the same behavior, in particular with -0 and 0?
There was a problem hiding this comment.
Thanks for bringing 0 versus -0 to the attention, Matt. In this particular case, FloatDifferenceULP returns an integer type. For integer types 0 and -0 are bit-wise equal, in C++. So exactly the same behavior, in this particular case.
Aims to make the code clearer. Cases found by regular expressions like
(\w+) = -\1;and\w+ \*= -1;Removed old comments from inside two
DistanceToLinemember functions, saying that they are "trying to avoid an expensive fabs".std::absisn't that expensive anymore nowadays.