Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Mar 26, 2024

Hello,

So, I worked on the fmaximum and fminimum functions recently and the reviewers suggested the structure:

if (bitsx ...)
  return ...;
if (bitsy ..)
  return 
...
return ...;

So I went ahead and did the same for fmin and fmax. I hope this isnt an issue for you all. thanks.

Hello,

So, I worked on the fmaximum and fminimum functions recently and the reviewers suggested the structure:

```
if (bitsx ...)
  return ...;
if (bitsy ..)
  return 
...
return ...;
```
So I went ahead and did the same for fmin and fmax. I hope this isnt an issue for you all. thanks.
@llvmbot llvmbot added the libc label Mar 26, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 26, 2024

@llvm/pr-subscribers-libc

Author: Job Henandez Lara (Jobhdez)

Changes

Hello,

So, I worked on the fmaximum and fminimum functions recently and the reviewers suggested the structure:

if (bitsx ...)
  return ...;
if (bitsy ..)
  return 
...
return ...;

So I went ahead and did the same for fmin and fmax. I hope this isnt an issue for you all. thanks.


Full diff: https://github.com/llvm/llvm-project/pull/86718.diff

1 Files Affected:

  • (modified) libc/src/__support/FPUtil/BasicOperations.h (+8-12)
diff --git a/libc/src/__support/FPUtil/BasicOperations.h b/libc/src/__support/FPUtil/BasicOperations.h
index 405755f8b57d9b..917b5ebc22be26 100644
--- a/libc/src/__support/FPUtil/BasicOperations.h
+++ b/libc/src/__support/FPUtil/BasicOperations.h
@@ -30,36 +30,32 @@ template <typename T, cpp::enable_if_t<cpp::is_floating_point_v<T>, int> = 0>
 LIBC_INLINE T fmin(T x, T y) {
   const FPBits<T> bitx(x), bity(y);
 
-  if (bitx.is_nan()) {
+  if (bitx.is_nan()) 
     return y;
-  } else if (bity.is_nan()) {
+  if (bity.is_nan()) 
     return x;
-  } else if (bitx.sign() != bity.sign()) {
+  if (bitx.sign() != bity.sign()) 
     // To make sure that fmin(+0, -0) == -0 == fmin(-0, +0), whenever x and
     // y has different signs and both are not NaNs, we return the number
     // with negative sign.
     return (bitx.is_neg()) ? x : y;
-  } else {
-    return (x < y ? x : y);
-  }
+  return (x < y ? x : y);
 }
 
 template <typename T, cpp::enable_if_t<cpp::is_floating_point_v<T>, int> = 0>
 LIBC_INLINE T fmax(T x, T y) {
   FPBits<T> bitx(x), bity(y);
 
-  if (bitx.is_nan()) {
+  if (bitx.is_nan()) 
     return y;
-  } else if (bity.is_nan()) {
+  if (bity.is_nan()) 
     return x;
-  } else if (bitx.sign() != bity.sign()) {
+  if (bitx.sign() != bity.sign()) 
     // To make sure that fmax(+0, -0) == +0 == fmax(-0, +0), whenever x and
     // y has different signs and both are not NaNs, we return the number
     // with positive sign.
     return (bitx.is_neg() ? y : x);
-  } else {
-    return (x > y ? x : y);
-  }
+  return (x > y ? x : y);
 }
 
 template <typename T, cpp::enable_if_t<cpp::is_floating_point_v<T>, int> = 0>

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct according to the LLVM coding standards so I'm in favor. I'll let some other libc people comment on whether or not libc adheres to that.

@jhuber6 jhuber6 changed the title refactor fmin and fmax [libc][NFC] refactor fmin and fmax Mar 26, 2024
Copy link

github-actions bot commented Mar 26, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@ghost
Copy link
Author

ghost commented Mar 26, 2024

This is correct according to the LLVM coding standards so I'm in favor. I'll let some other libc people comment on whether or not libc adheres to that.

sounds good

Copy link
Contributor

@lntue lntue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning this up!

@ghost
Copy link
Author

ghost commented Mar 26, 2024

Thanks for cleaning this up!

no problem!

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stuff is easy with suggestions.

@lntue lntue merged commit 056b404 into llvm:main Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants