[libc-commits] [libc] [libc] fix behavior of strrchr(x, '\0') (PR #112620)

[George Burgess IV via libc-commits]

https://github.com/gburgessiv created https://github.com/llvm/llvm-project/pull/112620

`strrchr("foo", '\0')` is defined to point to the end of `foo`, rather than returning NULL. This wasn't caught by tests, since llvm-libc's `ASSERT_STREQ(nullptr, "");` is not an assertion error.

While I'm here, refactor the test slightly to check for NULL more specifically. I considered adding fancier `ASSERT`s (and changing the semantics of `ASSERT_STREQ`), but opted for a more local fix by fair dice roll.

>From b2cba66f762ded233be31062e1536d75a528a8e3 Mon Sep 17 00:00:00 2001
From: George Burgess IV <george.burgess.iv at gmail.com>
Date: Wed, 16 Oct 2024 15:17:07 -0600
Subject: [PATCH] [libc] fix behavior of strrchr(x, '\0')

`strrchr("foo", '\0')` is defined to point to the end of `foo`, rather
than returning NULL. This wasn't caught by tests, since llvm-libc's
`ASSERT_STREQ(nullptr, "");` is not an assertion error.

While I'm here, refactor the test slightly to check for NULL more
specifically. I considered adding fancier `ASSERT`s (and changing the
semantics of `ASSERT_STREQ`), but opted for a more local fix by fair
dice roll.
---
 libc/src/string/string_utils.h    |  6 ++++--
 libc/test/src/string/StrchrTest.h | 32 +++++++++++++++++--------------
 2 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/libc/src/string/string_utils.h b/libc/src/string/string_utils.h
index 78381e46e480dd..90f6e4e710e5c4 100644
--- a/libc/src/string/string_utils.h
+++ b/libc/src/string/string_utils.h
@@ -239,11 +239,13 @@ LIBC_INLINE constexpr static char *strrchr_implementation(const char *src,
                                                           int c) {
   char ch = static_cast<char>(c);
   char *last_occurrence = nullptr;
-  for (; *src; ++src) {
+  while (true) {
     if (*src == ch)
       last_occurrence = const_cast<char *>(src);
+    if (!*src)
+      return last_occurrence;
+    ++src;
   }
-  return last_occurrence;
 }
 
 } // namespace internal
diff --git a/libc/test/src/string/StrchrTest.h b/libc/test/src/string/StrchrTest.h
index 74e172de95953e..c6267f042f59ea 100644
--- a/libc/test/src/string/StrchrTest.h
+++ b/libc/test/src/string/StrchrTest.h
@@ -40,14 +40,16 @@ template <auto Func> struct StrchrTest : public LIBC_NAMESPACE::testing::Test {
     const char *src = "abcde";
 
     // Should return null terminator.
-    ASSERT_STREQ(Func(src, '\0'), "");
+    const char *nul_terminator = Func(src, '\0');
+    ASSERT_TRUE(nul_terminator != nullptr);
+    ASSERT_STREQ(nul_terminator, "");
     // Source string should not change.
     ASSERT_STREQ(src, "abcde");
   }
 
   void characterNotWithinStringShouldReturnNullptr() {
     // Since 'z' is not within the string, should return nullptr.
-    ASSERT_STREQ(Func("123?", 'z'), nullptr);
+    ASSERT_TRUE(Func("123?", 'z') == nullptr);
   }
 
   void theSourceShouldNotChange() {
@@ -74,11 +76,12 @@ template <auto Func> struct StrchrTest : public LIBC_NAMESPACE::testing::Test {
 
   void emptyStringShouldOnlyMatchNullTerminator() {
     // Null terminator should match.
-    ASSERT_STREQ(Func("", '\0'), "");
+    const char empty_string[] = "";
+    ASSERT_TRUE(Func(empty_string, '\0') == empty_string);
     // All other characters should not match.
-    ASSERT_STREQ(Func("", 'Z'), nullptr);
-    ASSERT_STREQ(Func("", '3'), nullptr);
-    ASSERT_STREQ(Func("", '*'), nullptr);
+    ASSERT_TRUE(Func("", 'Z') == nullptr);
+    ASSERT_TRUE(Func("", '3') == nullptr);
+    ASSERT_TRUE(Func("", '*') == nullptr);
   }
 };
 
@@ -114,7 +117,7 @@ template <auto Func> struct StrrchrTest : public LIBC_NAMESPACE::testing::Test {
     const char *src = "abcde";
 
     // Should return null terminator.
-    ASSERT_STREQ(Func(src, '\0'), "");
+    ASSERT_TRUE(Func(src, '\0') != nullptr);
     // Source string should not change.
     ASSERT_STREQ(src, "abcde");
   }
@@ -122,9 +125,9 @@ template <auto Func> struct StrrchrTest : public LIBC_NAMESPACE::testing::Test {
   void findsLastBehindFirstNullTerminator() {
     static const char src[6] = {'a', 'a', '\0', 'b', '\0', 'c'};
     // 'b' is behind a null terminator, so should not be found.
-    ASSERT_STREQ(Func(src, 'b'), nullptr);
+    ASSERT_TRUE(Func(src, 'b') == nullptr);
     // Same goes for 'c'.
-    ASSERT_STREQ(Func(src, 'c'), nullptr);
+    ASSERT_TRUE(Func(src, 'c') == nullptr);
 
     // Should find the second of the two a's.
     ASSERT_STREQ(Func(src, 'a'), "a");
@@ -132,7 +135,7 @@ template <auto Func> struct StrrchrTest : public LIBC_NAMESPACE::testing::Test {
 
   void characterNotWithinStringShouldReturnNullptr() {
     // Since 'z' is not within the string, should return nullptr.
-    ASSERT_STREQ(Func("123?", 'z'), nullptr);
+    ASSERT_TRUE(Func("123?", 'z') == nullptr);
   }
 
   void shouldFindLastOfDuplicates() {
@@ -146,11 +149,12 @@ template <auto Func> struct StrrchrTest : public LIBC_NAMESPACE::testing::Test {
 
   void emptyStringShouldOnlyMatchNullTerminator() {
     // Null terminator should match.
-    ASSERT_STREQ(Func("", '\0'), "");
+    const char empty_string[] = "";
+    ASSERT_TRUE(Func(empty_string, '\0') == empty_string);
     // All other characters should not match.
-    ASSERT_STREQ(Func("", 'A'), nullptr);
-    ASSERT_STREQ(Func("", '2'), nullptr);
-    ASSERT_STREQ(Func("", '*'), nullptr);
+    ASSERT_TRUE(Func("", 'A') == nullptr);
+    ASSERT_TRUE(Func("", '2') == nullptr);
+    ASSERT_TRUE(Func("", '*') == nullptr);
   }
 };