Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 30, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions libc/src/string/string_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions libc/test/UnitTest/LibcTest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,14 @@ class Test {
(unsigned long long)RHS, LHSStr, RHSStr, Loc);
}

// Helper to allow macro invocations like `ASSERT_EQ(foo, nullptr)`.
template <typename ValType,
cpp::enable_if_t<cpp::is_pointer_v<ValType>, ValType> = nullptr>
bool test(TestCond Cond, ValType LHS, std::nullptr_t, const char *LHSStr,
Copy link
Member

@nickdesaulniers nickdesaulniers Oct 30, 2024

Choose a reason for hiding this comment

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

Looks like this is breaking the buildbots.

/home/libc_worker/libc-riscv64-debian/libc-riscv64-debian-fullbuild-dbg/llvm-project/libc/test/UnitTest/LibcTest.h:168:41: error: use of undeclared identifier 'std'
  bool test(TestCond Cond, ValType LHS, std::nullptr_t, const char *LHSStr,
                                        ^

mind reverting if you can't fix forward quickly?

Copy link
Member

@nickdesaulniers nickdesaulniers Oct 30, 2024

Choose a reason for hiding this comment

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

We have libc/src/__support/CPP/type_traits/is_null_pointer.h which defines cpp::nullptr_t. That include and that value should be used in place of std::nullptr_t I suspect.

Copy link
Contributor

@lntue lntue Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

pull/114321 should fix it

Copy link
Member Author

@gburgessiv gburgessiv Oct 30, 2024

Choose a reason for hiding this comment

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

Thanks for helping with this forward fix, and apologies for the breakage :)

const char *RHSStr, internal::Location Loc) {
return test(Cond, LHS, static_cast<ValType>(nullptr), LHSStr, RHSStr, Loc);
}

template <
typename ValType,
cpp::enable_if_t<
Expand Down
36 changes: 22 additions & 14 deletions libc/test/src/string/StrchrTest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_NE(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_EQ(Func("123?", 'z'), nullptr);
}

void theSourceShouldNotChange() {
Expand All @@ -74,11 +76,13 @@ 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_EQ(static_cast<const char *>(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_EQ(Func("", 'Z'), nullptr);
ASSERT_EQ(Func("", '3'), nullptr);
ASSERT_EQ(Func("", '*'), nullptr);
}
};

Expand Down Expand Up @@ -114,25 +118,27 @@ template <auto Func> struct StrrchrTest : 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_NE(nul_terminator, nullptr);
ASSERT_STREQ(nul_terminator, "");
// Source string should not change.
ASSERT_STREQ(src, "abcde");
}

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_EQ(Func(src, 'b'), nullptr);
// Same goes for 'c'.
ASSERT_STREQ(Func(src, 'c'), nullptr);
ASSERT_EQ(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);
ASSERT_EQ(Func("123?", 'z'), nullptr);
}

void shouldFindLastOfDuplicates() {
Expand All @@ -146,11 +152,13 @@ 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_EQ(static_cast<const char *>(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_EQ(Func("", 'A'), nullptr);
ASSERT_EQ(Func("", '2'), nullptr);
ASSERT_EQ(Func("", '*'), nullptr);
}
};

Expand Down
Loading