[libcxx-commits] [PATCH] D60097: Fix implementation of ::abs and std::abs LWG 2192.

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF marked an inline comment as done.
EricWF added a comment.

In D60097#1451656 <https://reviews.llvm.org/D60097#1451656>, @ldionne wrote:

> > Suggestions for a cleaner implementation are welcome.
>
> Why don't we move `abs` to a third header, say `__abs.h`, and then include that from both `math.h` and `stdlib.h`?


Because `__abs.h` will need to `#include_next` both `math.h` and `stdlib.h` to get the required declarations for `abs` and `fabs` anyway.
Which means we can't avoid the `_LIBCPP_STDLIB_INCLUDE_NEXT` trick.

And having `stdlib.h` drag in the C library `math.h` header but not our `math.h` header leaves us in a worse place. For example:

  #include <stdlib.h> // causes inclusion of /usr/include/math.h but not our math.h
  
  auto x = fabs(0.0f); // calls ::fabs(double)
  
  #include <math.h>
  
  auto y = fabs(0.0f); // calls ::fabs(float)

So I think adding an additional header doesn't solve any of our problems.



================
Comment at: include/math.h:800
+inline _LIBCPP_INLINE_VISIBILITY float abs(float __lcpp_x) _NOEXCEPT {
+  return __builtin_fabsf(__lcpp_x);
+}
----------------
ldionne wrote:
> mclow.lists wrote:
> > ldionne wrote:
> > > We were not using the builtin before -- why the change?
> > I agree. We can't use the intrinsics here w/o a check to see if the compiler supports them.
> > 
> > We'll probably want to use them in the future, when SG6 plasters `constexpr` all over `cmath`, but not right now.
> > I agree. We can't use the intrinsics here w/o a check to see if the compiler supports them.
> 
> Just to be clear -- this is not what I'm claiming (although you might be right), but I just wanted to understand the reason for this drive-by change.
> 
I think that's a remnant from an older version of this patch.

I'll remove the `__builtin_fabs` stuff.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60097/new/

https://reviews.llvm.org/D60097