From 82ca29ce54d3b7d8e47aa151e0ce3dff37727c1a Mon Sep 17 00:00:00 2001 From: Alex Brachet Date: Tue, 11 Apr 2023 01:11:32 +0000 Subject: [PATCH] [libc] Move str{,r}chr implementation to headers Differential Revision: https://reviews.llvm.org/D147463 --- libc/src/string/CMakeLists.txt | 4 + libc/src/string/strchr.cpp | 6 +- libc/src/string/strchrnul.cpp | 5 +- libc/src/string/string_utils.h | 19 ++ libc/src/string/strrchr.cpp | 9 +- libc/test/src/string/CMakeLists.txt | 8 + libc/test/src/string/StrchrTest.h | 201 +++++++++++++++++++++ libc/test/src/string/strchr_test.cpp | 74 +------- libc/test/src/string/strrchr_test.cpp | 72 +------- .../libc/test/src/string/BUILD.bazel | 8 + 10 files changed, 251 insertions(+), 155 deletions(-) create mode 100644 libc/test/src/string/StrchrTest.h diff --git a/libc/src/string/CMakeLists.txt b/libc/src/string/CMakeLists.txt index ee43c93..f6980ca 100644 --- a/libc/src/string/CMakeLists.txt +++ b/libc/src/string/CMakeLists.txt @@ -105,6 +105,8 @@ add_entrypoint_object( strchr.cpp HDRS strchr.h + DEPENDS + .string_utils ) add_entrypoint_object( @@ -323,6 +325,8 @@ add_entrypoint_object( strrchr.cpp HDRS strrchr.h + DEPENDS + .string_utils ) add_entrypoint_object( diff --git a/libc/src/string/strchr.cpp b/libc/src/string/strchr.cpp index 724c2a2..3b804e9 100644 --- a/libc/src/string/strchr.cpp +++ b/libc/src/string/strchr.cpp @@ -9,15 +9,13 @@ #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(c); - for (; *src && *src != ch; ++src) - ; - return *src == ch ? const_cast(src) : nullptr; + return internal::strchr_implementation(src, c); } } // namespace __llvm_libc diff --git a/libc/src/string/strchrnul.cpp b/libc/src/string/strchrnul.cpp index 4dc28cd..d8dce65 100644 --- a/libc/src/string/strchrnul.cpp +++ b/libc/src/string/strchrnul.cpp @@ -14,10 +14,7 @@ namespace __llvm_libc { LLVM_LIBC_FUNCTION(char *, strchrnul, (const char *src, int c)) { - const char ch = static_cast(c); - for (; *src && *src != ch; ++src) - ; - return const_cast(src); + return internal::strchr_implementation(src, c); } } // namespace __llvm_libc diff --git a/libc/src/string/string_utils.h b/libc/src/string/string_utils.h index c778ccf..572d47d 100644 --- a/libc/src/string/string_utils.h +++ b/libc/src/string/string_utils.h @@ -224,6 +224,25 @@ LIBC_INLINE size_t strlcpy(char *__restrict dst, const char *__restrict src, return len; } +template +constexpr static char *strchr_implementation(const char *src, int c) { + char ch = static_cast(c); + for (; *src && *src != ch; ++src) + ; + char *ret = ReturnNull ? nullptr : const_cast(src); + return *src == ch ? const_cast(src) : ret; +} + +constexpr static char *strrchr_implementation(const char *src, int c) { + char ch = static_cast(c); + char *last_occurrence = nullptr; + for (; *src; ++src) { + if (*src == ch) + last_occurrence = const_cast(src); + } + return last_occurrence; +} + } // namespace internal } // namespace __llvm_libc diff --git a/libc/src/string/strrchr.cpp b/libc/src/string/strrchr.cpp index 2e987fa..d821327 100644 --- a/libc/src/string/strrchr.cpp +++ b/libc/src/string/strrchr.cpp @@ -9,17 +9,12 @@ #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(c); - char *last_occurrence = nullptr; - for (; *src; ++src) { - if (*src == ch) - last_occurrence = const_cast(src); - } - return last_occurrence; + return internal::strrchr_implementation(src, c); } } // namespace __llvm_libc diff --git a/libc/test/src/string/CMakeLists.txt b/libc/test/src/string/CMakeLists.txt index 9d5933e..ed9d38a 100644 --- a/libc/test/src/string/CMakeLists.txt +++ b/libc/test/src/string/CMakeLists.txt @@ -84,6 +84,12 @@ add_libc_unittest( libc.src.string.strcat ) +add_header_library( + strchr_test_support + HDRS + StrchrTest.h +) + add_libc_unittest( strchr_test SUITE @@ -92,6 +98,7 @@ add_libc_unittest( strchr_test.cpp DEPENDS libc.src.string.strchr + .strchr_test_support ) add_libc_unittest( @@ -306,6 +313,7 @@ add_libc_unittest( strrchr_test.cpp DEPENDS libc.src.string.strrchr + .strchr_test_support ) add_libc_unittest( diff --git a/libc/test/src/string/StrchrTest.h b/libc/test/src/string/StrchrTest.h new file mode 100644 index 0000000..1c5bee5 --- /dev/null +++ b/libc/test/src/string/StrchrTest.h @@ -0,0 +1,201 @@ +//===-- 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 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 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; \ + 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; \ + 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(); \ + } diff --git a/libc/test/src/string/strchr_test.cpp b/libc/test/src/string/strchr_test.cpp index c619279..684b631 100644 --- a/libc/test/src/string/strchr_test.cpp +++ b/libc/test/src/string/strchr_test.cpp @@ -6,77 +6,9 @@ // //===----------------------------------------------------------------------===// +#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) diff --git a/libc/test/src/string/strrchr_test.cpp b/libc/test/src/string/strrchr_test.cpp index ba901cc..02659c0 100644 --- a/libc/test/src/string/strrchr_test.cpp +++ b/libc/test/src/string/strrchr_test.cpp @@ -6,75 +6,9 @@ // //===----------------------------------------------------------------------===// +#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) diff --git a/utils/bazel/llvm-project-overlay/libc/test/src/string/BUILD.bazel b/utils/bazel/llvm-project-overlay/libc/test/src/string/BUILD.bazel index 72b75a3..90ae8bb 100644 --- a/utils/bazel/llvm-project-overlay/libc/test/src/string/BUILD.bazel +++ b/utils/bazel/llvm-project-overlay/libc/test/src/string/BUILD.bazel @@ -42,12 +42,19 @@ libc_test( ], ) +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( @@ -80,6 +87,7 @@ libc_test( libc_function_deps = [ "//libc:strrchr", ], + deps = [":strchr_test_helper"], ) libc_test( -- 2.7.4