[libc] Move str{,r}chr implementation to headers

[michaelrj]

I'm not sure why this is necessary, given that only one function is calling the each of the strchr implementations, and they're each less than 10 lines of code. Do you plan to call these in more functions? If you are, would putting these implementations in string_utils.h make sense instead?

[abrachet]

In D147463#4241643, @michaelrj wrote:

I'm not sure why this is necessary, given that only one function is calling the each of the strchr implementations, and they're each less than 10 lines of code. Do you plan to call these in more functions? If you are, would putting these implementations in string_utils.h make sense instead?

For child revision D147464. Regarding string_utils.h, why is that preferred over memory_utils/? I'm happy to do it, I forgot that header existed which is why I added it to string_utils. But I am curious why it is preferred

[michaelrj]

Generally, memory_utils/ is used for optimized memory functions and is managed by @gchatelet. The string_utils.h file has the implementations for functions focused on null terminated strings and is managed by me.

Given that the followup patch does add more uses for these functions, I see this as a worthwhile change, though I'd suggest making the internal function work like strchrnul. That will let you implement both strchrnul and strchr with the same internal function and only minor additional code.

[gchatelet]

Given that the implementations are small and standard C++ I would move them as part string_utils rather than memory_utils as @michaelrj suggested.

libc/test/src/string/StrchrTest.h

158

It'd rather see templated tests rather than macros. They tend to be harder to debug and they impede readability and formatting.
If you follow @michaelrj advice you won't have to duplicate the tests.

[abrachet]

libc/test/src/string/StrchrTest.h

158

Are you referring to gtest typed tests?

[gchatelet]

libc/test/src/string/StrchrTest.h

158

Yes absolutely. I've used them in https://github.com/llvm/llvm-project/blob/main/libc/test/src/string/memory_utils/op_tests.cpp

[abrachet]

libc/test/src/string/StrchrTest.h

158

I guess I don't understand how that would look like other than something like:

strchr_test.cpp

#define FUNC Strchr

struct FuncT {
  static constexpr auto Func = __llvm_libc::strchr;
};

#include "StrchrTest.h"

and

StrchrTest.h

#define TEST_NAME_(LEFT, MIDDLE, RIGHT) LEFT ## RIGHT
#define TEST_NAME(NAME) TEST_NAME_(LlvmLibc, NAME, Test)

TYPED_TEST(TEST_NAME(FUNC), FindsFirstCharacter, FuncT) {
  constexpr auto Func = ParamType::Func;

  const char *src = "abcde";

  // Should return original string since 'a' is the first character.
  ASSERT_STREQ(Func(src, 'a'), "abcde");
  // Source string should not change.
  ASSERT_STREQ(src, "abcde");
}
...

Which I don't think is better. Unless you have something else in mind. This is what parameterized tests are for, but we don't have those.

FWIW, the current code is how we already do these kinds of test sharing, mostly in math/.

[gchatelet]

I was thinking something along the lines of https://reviews.llvm.org/D147690

[abrachet]

In D147463#4248131, @gchatelet wrote:

I was thinking something along the lines of https://reviews.llvm.org/D147690

I see. The reason I am partial to not doing that is because then to test strchr you must also include index, and it is very likely you would only want the former. I haven't included it in non linux targets, nor have I added it to the bazel build.

Moreover, the common structure of src/$func.{cpp,h} and test/src/$func_test.cpp makes downstream build infrastructure much easier when all functions follow the same pattern than multiple functions sharing a single test file.

[abrachet]

ping

We would like to get D147464 landed soon

[michaelrj]

While I like the idea of avoiding macros, I think it's more important for the tests to be independent of which entrypoints are included. This patch looks good to me.

[mcgrathr]

libc/test/src/string/StrchrTest.h

158

TYPED_TEST is problematic since it's not supported by the other compatible-enough test frameworks where these tests are built, such as Fuchsia's zxtest. I think the tolerable middle ground that makes the boilerplate at least less annoying while avoiding macros is to rely on the fixture as done here to keep the actual test logic common and just template-parameterized with the actual function pointer, but then write out both copies of all the TEST_F(LlvmLibcStrchrTest, ...) and TEST_F(LlvmLibcIndexTest, ...). We can see in the future if using TYPED_TEST here can be made feasible, but it's just not yet.

I'd like to get this unblocked and land some version of this so we can get index and rindex landed.

[gchatelet]

Sorry for the delayed response.
I missed the requirement that the tests should be independent. Given that the build system decides which functions are to be tested, it's difficult to use the templated approach I was suggested.

LGTM

libc/src/string/CMakeLists.txt

 )
 
 add_entrypoint_object(
   strchr
   SRCS
     strchr.cpp
   HDRS
     strchr.h
+  DEPENDS
+    .string_utils
 )
 
 add_entrypoint_object(
   strchrnul
   SRCS
     strchrnul.cpp
   HDRS
     strchrnul.h

 )
 
 add_entrypoint_object(
   strrchr
   SRCS
     strrchr.cpp
   HDRS
     strrchr.h
+  DEPENDS
+    .string_utils
 )
 
 add_entrypoint_object(
   strsep
   SRCS
     strsep.cpp
   HDRS
     strsep.h

libc/src/string/strchr.cpp

 //===-- Implementation of strchr ------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "src/string/strchr.h"
 
 #include "src/__support/common.h"
+#include "src/string/string_utils.h"
 
 namespace __llvm_libc {
 
 // TODO: Look at performance benefits of comparing words.
 LLVM_LIBC_FUNCTION(char *, strchr, (const char *src, int c)) {
-  const char ch = static_cast<char>(c);
-  for (; *src && *src != ch; ++src)
-    ;
-  return *src == ch ? const_cast<char *>(src) : nullptr;
+  return internal::strchr_implementation(src, c);
 }
 
 } // namespace __llvm_libc

libc/src/string/strchrnul.cpp

 //===-- Implementation of strchrnul --------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "src/string/strchrnul.h"
 #include "src/string/string_utils.h"
 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(char *, strchrnul, (const char *src, int c)) {
-  const char ch = static_cast<char>(c);
-  for (; *src && *src != ch; ++src)
-    ;
-  return const_cast<char *>(src);
+  return internal::strchr_implementation<false>(src, c);
 }
 
 } // namespace __llvm_libc

libc/src/string/string_utils.h

LIBC_INLINE size_t strlcpy(char *__restrict dst, const char *__restrict src,
   if (!size)
     return len;
   size_t n = len < size - 1 ? len : size - 1;
   inline_memcpy(dst, src, n);
   inline_bzero(dst + n, size - n);
   return len;
 }
 
+template <bool ReturnNull = true>
+constexpr static char *strchr_implementation(const char *src, int c) {
+  char ch = static_cast<char>(c);
+  for (; *src && *src != ch; ++src)
+    ;
+  char *ret = ReturnNull ? nullptr : const_cast<char *>(src);
+  return *src == ch ? const_cast<char *>(src) : ret;
+}
+
+constexpr static char *strrchr_implementation(const char *src, int c) {
+  char ch = static_cast<char>(c);
+  char *last_occurrence = nullptr;
+  for (; *src; ++src) {
+    if (*src == ch)
+      last_occurrence = const_cast<char *>(src);
+  }
+  return last_occurrence;
+}
+
 } // namespace internal
 } // namespace __llvm_libc
 
 #endif //  LIBC_SRC_STRING_STRING_UTILS_H

libc/src/string/strrchr.cpp

 //===-- Implementation of strrchr------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "src/string/strrchr.h"
 
 #include "src/__support/common.h"
+#include "src/string/string_utils.h"
 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(char *, strrchr, (const char *src, int c)) {
-  const char ch = static_cast<char>(c);
-  char *last_occurrence = nullptr;
-  for (; *src; ++src) {
-    if (*src == ch)
-      last_occurrence = const_cast<char *>(src);
-  }
-  return last_occurrence;
+  return internal::strrchr_implementation(src, c);
 }
 
 } // namespace __llvm_libc

libc/test/src/string/CMakeLists.txt

add_libc_unittest(
   SUITE
     libc_string_unittests
   SRCS
     strcat_test.cpp
   DEPENDS
     libc.src.string.strcat
 )
 
+add_header_library(
+  strchr_test_support
+  HDRS
+    StrchrTest.h
+)
+
 add_libc_unittest(
   strchr_test
   SUITE
     libc_string_unittests
   SRCS
     strchr_test.cpp
   DEPENDS
     libc.src.string.strchr
+    .strchr_test_support
 )
 
 add_libc_unittest(
   strchrnul_test
   SUITE
     libc_string_unittests
   SRCS
     strchrnul_test.cpp

 add_libc_unittest(
   strrchr_test
   SUITE
     libc_string_unittests
   SRCS
     strrchr_test.cpp
   DEPENDS
     libc.src.string.strrchr
+    .strchr_test_support
 )
 
 add_libc_unittest(
   strsep_test
   SUITE
     libc_string_unittests
   SRCS
     strsep_test.cpp

libc/test/src/string/StrchrTest.h

+//===-- Tests for str{,r}chr and {,r}index functions ------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "test/UnitTest/Test.h"
+
+template <auto Func> struct StrchrTest : public __llvm_libc::testing::Test {
+  void findsFirstCharacter() {
+    const char *src = "abcde";
+
+    // Should return original string since 'a' is the first character.
+    ASSERT_STREQ(Func(src, 'a'), "abcde");
+    // Source string should not change.
+    ASSERT_STREQ(src, "abcde");
+  }
+
+  void findsMiddleCharacter() {
+    const char *src = "abcde";
+
+    // Should return characters after (and including) 'c'.
+    ASSERT_STREQ(Func(src, 'c'), "cde");
+    // Source string should not change.
+    ASSERT_STREQ(src, "abcde");
+  }
+
+  void findsLastCharacterThatIsNotNullTerminator() {
+    const char *src = "abcde";
+
+    // Should return 'e' and null-terminator.
+    ASSERT_STREQ(Func(src, 'e'), "e");
+    // Source string should not change.
+    ASSERT_STREQ(src, "abcde");
+  }
+
+  void findsNullTerminator() {
+    const char *src = "abcde";
+
+    // Should return null terminator.
+    ASSERT_STREQ(Func(src, '\0'), "");
+    // 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);
+  }
+
+  void theSourceShouldNotChange() {
+    const char *src = "abcde";
+    // When the character is found, the source string should not change.
+    Func(src, 'd');
+    ASSERT_STREQ(src, "abcde");
+    // Same case for when the character is not found.
+    Func(src, 'z');
+    ASSERT_STREQ(src, "abcde");
+    // Same case for when looking for nullptr.
+    Func(src, '\0');
+    ASSERT_STREQ(src, "abcde");
+  }
+
+  void shouldFindFirstOfDuplicates() {
+    // '1' is duplicated in the string, but it should find the first copy.
+    ASSERT_STREQ(Func("abc1def1ghi", '1'), "1def1ghi");
+
+    const char *dups = "XXXXX";
+    // Should return original string since 'X' is the first character.
+    ASSERT_STREQ(Func(dups, 'X'), dups);
+  }
+
+  void emptyStringShouldOnlyMatchNullTerminator() {
+    // Null terminator should match.
+    ASSERT_STREQ(Func("", '\0'), "");
+    // All other characters should not match.
+    ASSERT_STREQ(Func("", 'Z'), nullptr);
+    ASSERT_STREQ(Func("", '3'), nullptr);
+    ASSERT_STREQ(Func("", '*'), nullptr);
+  }
+};
+
+template <auto Func> struct StrrchrTest : public __llvm_libc::testing::Test {
+  void findsFirstCharacter() {
+    const char *src = "abcde";
+
+    // Should return original string since 'a' is the first character.
+    ASSERT_STREQ(Func(src, 'a'), "abcde");
+    // Source string should not change.
+    ASSERT_STREQ(src, "abcde");
+  }
+
+  void findsMiddleCharacter() {
+    const char *src = "abcde";
+
+    // Should return characters after (and including) 'c'.
+    ASSERT_STREQ(Func(src, 'c'), "cde");
+    // Source string should not change.
+    ASSERT_STREQ(src, "abcde");
+  }
+
+  void findsLastCharacterThatIsNotNullTerminator() {
+    const char *src = "abcde";
+
+    // Should return 'e' and null-terminator.
+    ASSERT_STREQ(Func(src, 'e'), "e");
+    // Source string should not change.
+    ASSERT_STREQ(src, "abcde");
+  }
+
+  void findsNullTerminator() {
+    const char *src = "abcde";
+
+    // Should return null terminator.
+    ASSERT_STREQ(Func(src, '\0'), "");
+    // Source string should not change.
+    ASSERT_STREQ(src, "abcde");
+  }
+
+  void findsLastBehindFirstNullTerminator() {
+    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);
+    // Same goes for 'c'.
+    ASSERT_STREQ(Func(src, 'c'), nullptr);
+
+    // Should find the second of the two a's.
+    ASSERT_STREQ(Func(src, 'a'), "a");
+  }
+
+  void characterNotWithinStringShouldReturnNullptr() {
+    // Since 'z' is not within the string, should return nullptr.
+    ASSERT_STREQ(Func("123?", 'z'), nullptr);
+  }
+
+  void shouldFindLastOfDuplicates() {
+    // '1' is duplicated in the string, but it should find the last copy.
+    ASSERT_STREQ(Func("abc1def1ghi", '1'), "1ghi");
+
+    const char *dups = "XXXXX";
+    // Should return the last occurrence of 'X'.
+    ASSERT_STREQ(Func(dups, 'X'), "X");
+  }
+
+  void emptyStringShouldOnlyMatchNullTerminator() {
+    // Null terminator should match.
+    ASSERT_STREQ(Func("", '\0'), "");
+    // All other characters should not match.
+    ASSERT_STREQ(Func("", 'A'), nullptr);
+    ASSERT_STREQ(Func("", '2'), nullptr);
+    ASSERT_STREQ(Func("", '*'), nullptr);
+  }
+};
+
+#define STRCHR_TEST(name, func)                                                \
+  using LlvmLibc##name##Test = StrchrTest<func>;                               \
+  TEST_F(LlvmLibc##name##Test, FindsFirstCharacter) { findsFirstCharacter(); } \
+  TEST_F(LlvmLibc##name##Test, FindsMiddleCharacter) {                         \
+    findsMiddleCharacter();                                                    \
+  }                                                                            \
+  TEST_F(LlvmLibc##name##Test, FindsLastCharacterThatIsNotNullTerminator) {    \
+    findsLastCharacterThatIsNotNullTerminator();                               \
+  }                                                                            \
+  TEST_F(LlvmLibc##name##Test, FindsNullTerminator) { findsNullTerminator(); } \
+  TEST_F(LlvmLibc##name##Test, CharacterNotWithinStringShouldReturnNullptr) {  \
+    characterNotWithinStringShouldReturnNullptr();                             \
+  }                                                                            \
+  TEST_F(LlvmLibc##name##Test, TheSourceShouldNotChange) {                     \
+    theSourceShouldNotChange();                                                \
+  }                                                                            \
+  TEST_F(LlvmLibc##name##Test, ShouldFindFirstOfDuplicates) {                  \
+    shouldFindFirstOfDuplicates();                                             \
+  }                                                                            \
+  TEST_F(LlvmLibc##name##Test, EmptyStringShouldOnlyMatchNullTerminator) {     \
+    emptyStringShouldOnlyMatchNullTerminator();                                \
+  }
+
+#define STRRCHR_TEST(name, func)                                               \
+  using LlvmLibc##name##Test = StrrchrTest<func>;                              \
+  TEST_F(LlvmLibc##name##Test, FindsFirstCharacter) { findsFirstCharacter(); } \
+  TEST_F(LlvmLibc##name##Test, FindsMiddleCharacter) {                         \
+    findsMiddleCharacter();                                                    \
+  }                                                                            \
+  TEST_F(LlvmLibc##name##Test, FindsLastCharacterThatIsNotNullTerminator) {    \
+    findsLastCharacterThatIsNotNullTerminator();                               \
+  }                                                                            \
+  TEST_F(LlvmLibc##name##Test, FindsNullTerminator) { findsNullTerminator(); } \
+  TEST_F(LlvmLibc##name##Test, FindsLastBehindFirstNullTerminator) {           \
+    findsLastBehindFirstNullTerminator();                                      \
+  }                                                                            \
+  TEST_F(LlvmLibc##name##Test, CharacterNotWithinStringShouldReturnNullptr) {  \
+    characterNotWithinStringShouldReturnNullptr();                             \
+  }                                                                            \
+  TEST_F(LlvmLibc##name##Test, ShouldFindLastOfDuplicates) {                   \
+    shouldFindLastOfDuplicates();                                              \
+  }                                                                            \
+  TEST_F(LlvmLibc##name##Test, EmptyStringShouldOnlyMatchNullTerminator) {     \
+    emptyStringShouldOnlyMatchNullTerminator();                                \
+  }

libc/test/src/string/strchr_test.cpp

 //===-- Unittests for strchr ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "StrchrTest.h"
+
 #include "src/string/strchr.h"
 #include "test/UnitTest/Test.h"
 
-TEST(LlvmLibcStrChrTest, FindsFirstCharacter) {
-  const char *src = "abcde";
-
-  // Should return original string since 'a' is the first character.
-  ASSERT_STREQ(__llvm_libc::strchr(src, 'a'), "abcde");
-  // Source string should not change.
-  ASSERT_STREQ(src, "abcde");
-}
-
-TEST(LlvmLibcStrChrTest, FindsMiddleCharacter) {
-  const char *src = "abcde";
-
-  // Should return characters after (and including) 'c'.
-  ASSERT_STREQ(__llvm_libc::strchr(src, 'c'), "cde");
-  // Source string should not change.
-  ASSERT_STREQ(src, "abcde");
-}
-
-TEST(LlvmLibcStrChrTest, FindsLastCharacterThatIsNotNullTerminator) {
-  const char *src = "abcde";
-
-  // Should return 'e' and null-terminator.
-  ASSERT_STREQ(__llvm_libc::strchr(src, 'e'), "e");
-  // Source string should not change.
-  ASSERT_STREQ(src, "abcde");
-}
-
-TEST(LlvmLibcStrChrTest, FindsNullTerminator) {
-  const char *src = "abcde";
-
-  // Should return null terminator.
-  ASSERT_STREQ(__llvm_libc::strchr(src, '\0'), "");
-  // Source string should not change.
-  ASSERT_STREQ(src, "abcde");
-}
-
-TEST(LlvmLibcStrChrTest, CharacterNotWithinStringShouldReturnNullptr) {
-  // Since 'z' is not within the string, should return nullptr.
-  ASSERT_STREQ(__llvm_libc::strchr("123?", 'z'), nullptr);
-}
-
-TEST(LlvmLibcStrChrTest, TheSourceShouldNotChange) {
-  const char *src = "abcde";
-  // When the character is found, the source string should not change.
-  __llvm_libc::strchr(src, 'd');
-  ASSERT_STREQ(src, "abcde");
-  // Same case for when the character is not found.
-  __llvm_libc::strchr(src, 'z');
-  ASSERT_STREQ(src, "abcde");
-  // Same case for when looking for null terminator.
-  __llvm_libc::strchr(src, '\0');
-  ASSERT_STREQ(src, "abcde");
-}
-
-TEST(LlvmLibcStrChrTest, ShouldFindFirstOfDuplicates) {
-  // '1' is duplicated in the string, but it should find the first copy.
-  ASSERT_STREQ(__llvm_libc::strchr("abc1def1ghi", '1'), "1def1ghi");
-
-  const char *dups = "XXXXX";
-  // Should return original string since 'X' is the first character.
-  ASSERT_STREQ(__llvm_libc::strchr(dups, 'X'), dups);
-}
-
-TEST(LlvmLibcStrChrTest, EmptyStringShouldOnlyMatchNullTerminator) {
-  // Null terminator should match.
-  ASSERT_STREQ(__llvm_libc::strchr("", '\0'), "");
-  // All other characters should not match.
-  ASSERT_STREQ(__llvm_libc::strchr("", 'Z'), nullptr);
-  ASSERT_STREQ(__llvm_libc::strchr("", '3'), nullptr);
-  ASSERT_STREQ(__llvm_libc::strchr("", '*'), nullptr);
-}
+STRCHR_TEST(Strchr, __llvm_libc::strchr)

libc/test/src/string/strrchr_test.cpp

 //===-- Unittests for strrchr ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "StrchrTest.h"
+
 #include "src/string/strrchr.h"
 #include "test/UnitTest/Test.h"
 
-TEST(LlvmLibcStrRChrTest, FindsFirstCharacter) {
-  const char *src = "abcde";
-
-  // Should return original string since 'a' is the first character.
-  ASSERT_STREQ(__llvm_libc::strrchr(src, 'a'), "abcde");
-  // Source string should not change.
-  ASSERT_STREQ(src, "abcde");
-}
-
-TEST(LlvmLibcStrRChrTest, FindsMiddleCharacter) {
-  const char *src = "abcde";
-
-  // Should return characters after (and including) 'c'.
-  ASSERT_STREQ(__llvm_libc::strrchr(src, 'c'), "cde");
-  // Source string should not change.
-  ASSERT_STREQ(src, "abcde");
-}
-
-TEST(LlvmLibcStrRChrTest, FindsLastCharacterThatIsNotNullTerminator) {
-  const char *src = "abcde";
-
-  // Should return 'e' and null-terminator.
-  ASSERT_STREQ(__llvm_libc::strrchr(src, 'e'), "e");
-  // Source string should not change.
-  ASSERT_STREQ(src, "abcde");
-}
-
-TEST(LlvmLibcStrRChrTest, FindsNullTerminator) {
-  const char *src = "abcde";
-
-  // Should return null terminator.
-  ASSERT_STREQ(__llvm_libc::strrchr(src, '\0'), "");
-  // Source string should not change.
-  ASSERT_STREQ(src, "abcde");
-}
-
-TEST(LlvmLibcStrRChrTest, FindsLastBehindFirstNullTerminator) {
-  const char src[6] = {'a', 'a', '\0', 'b', '\0', 'c'};
-  // 'b' is behind a null terminator, so should not be found.
-  ASSERT_STREQ(__llvm_libc::strrchr(src, 'b'), nullptr);
-  // Same goes for 'c'.
-  ASSERT_STREQ(__llvm_libc::strrchr(src, 'c'), nullptr);
-  
-  // Should find the second of the two a's.
-  ASSERT_STREQ(__llvm_libc::strrchr(src, 'a'), "a");
-}
-
-TEST(LlvmLibcStrRChrTest, CharacterNotWithinStringShouldReturnNullptr) {
-  // Since 'z' is not within the string, should return nullptr.
-  ASSERT_STREQ(__llvm_libc::strrchr("123?", 'z'), nullptr);
-}
-
-TEST(LlvmLibcStrRChrTest, ShouldFindLastOfDuplicates) {
-  // '1' is duplicated in the string, but it should find the last copy.
-  ASSERT_STREQ(__llvm_libc::strrchr("abc1def1ghi", '1'), "1ghi");
-
-  const char *dups = "XXXXX";
-  // Should return the last occurrence of 'X'.
-  ASSERT_STREQ(__llvm_libc::strrchr(dups, 'X'), "X");
-}
-
-TEST(LlvmLibcStrRChrTest, EmptyStringShouldOnlyMatchNullTerminator) {
-  // Null terminator should match.
-  ASSERT_STREQ(__llvm_libc::strrchr("", '\0'), "");
-  // All other characters should not match.
-  ASSERT_STREQ(__llvm_libc::strrchr("", 'A'), nullptr);
-  ASSERT_STREQ(__llvm_libc::strrchr("", '2'), nullptr);
-  ASSERT_STREQ(__llvm_libc::strrchr("", '*'), nullptr);
-}
+STRRCHR_TEST(Strrchr, __llvm_libc::strrchr)

utils/bazel/llvm-project-overlay/libc/test/src/string/BUILD.bazel

 libc_test(
     name = "memchr_test",
     srcs = ["memchr_test.cpp"],
     libc_function_deps = [
         "//libc:memchr",
     ],
 )
 
+cc_library(
+    name = "strchr_test_helper",
+    hdrs = ["StrchrTest.h"],
+    deps = ["//libc/test/UnitTest:LibcUnitTest"],
+)
+
 libc_test(
     name = "strchr_test",
     srcs = ["strchr_test.cpp"],
     libc_function_deps = [
         "//libc:strchr",
     ],
+    deps = [":strchr_test_helper"],
 )
 
 libc_test(
     name = "strstr_test",
     srcs = ["strstr_test.cpp"],
     libc_function_deps = [
         "//libc:strstr",
     ],

 )
 
 libc_test(
     name = "strrchr_test",
     srcs = ["strrchr_test.cpp"],
     libc_function_deps = [
         "//libc:strrchr",
     ],
+    deps = [":strchr_test_helper"],
 )
 
 libc_test(
     name = "strcspn_test",
     srcs = ["strcspn_test.cpp"],
     libc_function_deps = [
         "//libc:strcspn",
     ],