[libc-commits] [libc] [libc] Make `stdc_first_trailing_one` spec compliant (PR #116493)

[via libc-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libc

Author: strncmp (strncmp)

<details>
<summary>Changes</summary>

# A small description
`stdc_first_trailing_one` was exhibiting non-compliant behaviour in the edge cases of the type it was operating on. More specifically, when the user passes 0, the function shall return 0, since none of the bits can be asserted. The previous revision implemented this behaviour for the max value of the type instead, making the implementation non-compliant.
Spec reference -> N3220: 7.18.10 First Trailing One

# Issue
Fixes #<!-- -->115719 

Please note that this is my first contribution and therefore I don't have commit access

# Review request
cc @<!-- -->nickdesaulniers @<!-- -->michaelrj-google 

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


8 Files Affected:

- (modified) libc/src/__support/big_int.h (+1-2) 
- (modified) libc/src/__support/math_extras.h (+1-2) 
- (modified) libc/test/src/__support/math_extras_test.cpp (+1-1) 
- (modified) libc/test/src/stdbit/stdc_first_trailing_one_uc_test.cpp (+1-1) 
- (modified) libc/test/src/stdbit/stdc_first_trailing_one_ui_test.cpp (+1-1) 
- (modified) libc/test/src/stdbit/stdc_first_trailing_one_ul_test.cpp (+1-1) 
- (modified) libc/test/src/stdbit/stdc_first_trailing_one_ull_test.cpp (+1-1) 
- (modified) libc/test/src/stdbit/stdc_first_trailing_one_us_test.cpp (+1-1) 


``````````diff
diff --git a/libc/src/__support/big_int.h b/libc/src/__support/big_int.h
index a95ab4ff8e1abf..3745a2c0017b31 100644
--- a/libc/src/__support/big_int.h
+++ b/libc/src/__support/big_int.h
@@ -1375,8 +1375,7 @@ first_trailing_zero(T value) {
 template <typename T>
 [[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<is_big_int_v<T>, int>
 first_trailing_one(T value) {
-  return value == cpp::numeric_limits<T>::max() ? 0
-                                                : cpp::countr_zero(value) + 1;
+  return first_trailing_zero(~value);
 }
 
 } // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/__support/math_extras.h b/libc/src/__support/math_extras.h
index 6f4a006aad2700..dded3f9352be33 100644
--- a/libc/src/__support/math_extras.h
+++ b/libc/src/__support/math_extras.h
@@ -146,8 +146,7 @@ first_trailing_zero(T value) {
 template <typename T>
 [[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
 first_trailing_one(T value) {
-  return value == cpp::numeric_limits<T>::max() ? 0
-                                                : cpp::countr_zero(value) + 1;
+  return value == T(0) ? 0 : cpp::countr_zero(value) + 1;
 }
 
 template <typename T>
diff --git a/libc/test/src/__support/math_extras_test.cpp b/libc/test/src/__support/math_extras_test.cpp
index 08c090017c1a19..a45c7851328ee0 100644
--- a/libc/test/src/__support/math_extras_test.cpp
+++ b/libc/test/src/__support/math_extras_test.cpp
@@ -91,7 +91,7 @@ TYPED_TEST(LlvmLibcBitTest, FirstTrailingZero, UnsignedTypesNoBigInt) {
 }
 
 TYPED_TEST(LlvmLibcBitTest, FirstTrailingOne, UnsignedTypesNoBigInt) {
-  EXPECT_EQ(first_trailing_one<T>(cpp::numeric_limits<T>::max()), 0);
+  EXPECT_EQ(first_trailing_one<T>(T(0)), 0);
   for (int i = 0U; i != cpp::numeric_limits<T>::digits; ++i)
     EXPECT_EQ(first_trailing_one<T>(T(1) << i), i + 1);
 }
diff --git a/libc/test/src/stdbit/stdc_first_trailing_one_uc_test.cpp b/libc/test/src/stdbit/stdc_first_trailing_one_uc_test.cpp
index ed2b4921cdada4..9f481e40cee496 100644
--- a/libc/test/src/stdbit/stdc_first_trailing_one_uc_test.cpp
+++ b/libc/test/src/stdbit/stdc_first_trailing_one_uc_test.cpp
@@ -11,7 +11,7 @@
 #include "test/UnitTest/Test.h"
 
 TEST(LlvmLibcStdcFirstTrailingOneUcTest, ALL) {
-  EXPECT_EQ(LIBC_NAMESPACE::stdc_first_trailing_one_uc(UCHAR_MAX), 0U);
+  EXPECT_EQ(LIBC_NAMESPACE::stdc_first_trailing_one_uc(0), 0U);
 }
 
 TEST(LlvmLibcStdcFirstTrailingOneUcTest, OneHot) {
diff --git a/libc/test/src/stdbit/stdc_first_trailing_one_ui_test.cpp b/libc/test/src/stdbit/stdc_first_trailing_one_ui_test.cpp
index 137c8a42e407df..b3415d552a0f35 100644
--- a/libc/test/src/stdbit/stdc_first_trailing_one_ui_test.cpp
+++ b/libc/test/src/stdbit/stdc_first_trailing_one_ui_test.cpp
@@ -11,7 +11,7 @@
 #include "test/UnitTest/Test.h"
 
 TEST(LlvmLibcStdcFirstTrailingOneUiTest, ALL) {
-  EXPECT_EQ(LIBC_NAMESPACE::stdc_first_trailing_one_ui(UINT_MAX), 0U);
+  EXPECT_EQ(LIBC_NAMESPACE::stdc_first_trailing_one_ui(0U), 0U);
 }
 
 TEST(LlvmLibcStdcFirstTrailingOneUiTest, OneHot) {
diff --git a/libc/test/src/stdbit/stdc_first_trailing_one_ul_test.cpp b/libc/test/src/stdbit/stdc_first_trailing_one_ul_test.cpp
index 3fc1f3f16c60de..8fd7141862803c 100644
--- a/libc/test/src/stdbit/stdc_first_trailing_one_ul_test.cpp
+++ b/libc/test/src/stdbit/stdc_first_trailing_one_ul_test.cpp
@@ -11,7 +11,7 @@
 #include "test/UnitTest/Test.h"
 
 TEST(LlvmLibcStdcFirstTrailingOneUlTest, ALL) {
-  EXPECT_EQ(LIBC_NAMESPACE::stdc_first_trailing_one_ul(ULONG_MAX), 0U);
+  EXPECT_EQ(LIBC_NAMESPACE::stdc_first_trailing_one_ul(0UL), 0U);
 }
 
 TEST(LlvmLibcStdcFirstTrailingOneUlTest, OneHot) {
diff --git a/libc/test/src/stdbit/stdc_first_trailing_one_ull_test.cpp b/libc/test/src/stdbit/stdc_first_trailing_one_ull_test.cpp
index 5719e09a5120a0..ad661bd9bf7bc0 100644
--- a/libc/test/src/stdbit/stdc_first_trailing_one_ull_test.cpp
+++ b/libc/test/src/stdbit/stdc_first_trailing_one_ull_test.cpp
@@ -11,7 +11,7 @@
 #include "test/UnitTest/Test.h"
 
 TEST(LlvmLibcStdcFirstTrailingOneUllTest, ALL) {
-  EXPECT_EQ(LIBC_NAMESPACE::stdc_first_trailing_one_ull(ULLONG_MAX), 0U);
+  EXPECT_EQ(LIBC_NAMESPACE::stdc_first_trailing_one_ull(0ULL), 0U);
 }
 
 TEST(LlvmLibcStdcFirstTrailingOneUllTest, OneHot) {
diff --git a/libc/test/src/stdbit/stdc_first_trailing_one_us_test.cpp b/libc/test/src/stdbit/stdc_first_trailing_one_us_test.cpp
index 60021552310bee..674bdd2f0bb33d 100644
--- a/libc/test/src/stdbit/stdc_first_trailing_one_us_test.cpp
+++ b/libc/test/src/stdbit/stdc_first_trailing_one_us_test.cpp
@@ -11,7 +11,7 @@
 #include "test/UnitTest/Test.h"
 
 TEST(LlvmLibcStdcFirstTrailingOneUsTest, ALL) {
-  EXPECT_EQ(LIBC_NAMESPACE::stdc_first_trailing_one_us(USHRT_MAX), 0U);
+  EXPECT_EQ(LIBC_NAMESPACE::stdc_first_trailing_one_us(0), 0U);
 }
 
 TEST(LlvmLibcStdcFirstTrailingOneUsTest, OneHot) {

``````````

</details>


https://github.com/llvm/llvm-project/pull/116493