Skip to content

Commit 0d3e459

Browse files
committed
[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.
1 parent 7033408 commit 0d3e459

File tree

2 files changed

+24
-16
lines changed

2 files changed

+24
-16
lines changed

libc/src/string/string_utils.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -239,11 +239,13 @@ LIBC_INLINE constexpr static char *strrchr_implementation(const char *src,
239239
int c) {
240240
char ch = static_cast<char>(c);
241241
char *last_occurrence = nullptr;
242-
for (; *src; ++src) {
242+
while (true) {
243243
if (*src == ch)
244244
last_occurrence = const_cast<char *>(src);
245+
if (!*src)
246+
return last_occurrence;
247+
++src;
245248
}
246-
return last_occurrence;
247249
}
248250

249251
} // namespace internal

libc/test/src/string/StrchrTest.h

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -40,14 +40,16 @@ template <auto Func> struct StrchrTest : public LIBC_NAMESPACE::testing::Test {
4040
const char *src = "abcde";
4141

4242
// Should return null terminator.
43-
ASSERT_STREQ(Func(src, '\0'), "");
43+
const char *nul_terminator = Func(src, '\0');
44+
ASSERT_TRUE(nul_terminator != nullptr);
45+
ASSERT_STREQ(nul_terminator, "");
4446
// Source string should not change.
4547
ASSERT_STREQ(src, "abcde");
4648
}
4749

4850
void characterNotWithinStringShouldReturnNullptr() {
4951
// Since 'z' is not within the string, should return nullptr.
50-
ASSERT_STREQ(Func("123?", 'z'), nullptr);
52+
ASSERT_TRUE(Func("123?", 'z') == nullptr);
5153
}
5254

5355
void theSourceShouldNotChange() {
@@ -74,11 +76,12 @@ template <auto Func> struct StrchrTest : public LIBC_NAMESPACE::testing::Test {
7476

7577
void emptyStringShouldOnlyMatchNullTerminator() {
7678
// Null terminator should match.
77-
ASSERT_STREQ(Func("", '\0'), "");
79+
const char empty_string[] = "";
80+
ASSERT_TRUE(Func(empty_string, '\0') == empty_string);
7881
// All other characters should not match.
79-
ASSERT_STREQ(Func("", 'Z'), nullptr);
80-
ASSERT_STREQ(Func("", '3'), nullptr);
81-
ASSERT_STREQ(Func("", '*'), nullptr);
82+
ASSERT_TRUE(Func("", 'Z') == nullptr);
83+
ASSERT_TRUE(Func("", '3') == nullptr);
84+
ASSERT_TRUE(Func("", '*') == nullptr);
8285
}
8386
};
8487

@@ -114,25 +117,27 @@ template <auto Func> struct StrrchrTest : public LIBC_NAMESPACE::testing::Test {
114117
const char *src = "abcde";
115118

116119
// Should return null terminator.
117-
ASSERT_STREQ(Func(src, '\0'), "");
120+
const char *nul_terminator = Func(src, '\0');
121+
ASSERT_TRUE(nul_terminator != nullptr);
122+
ASSERT_STREQ(nul_terminator, "");
118123
// Source string should not change.
119124
ASSERT_STREQ(src, "abcde");
120125
}
121126

122127
void findsLastBehindFirstNullTerminator() {
123128
static const char src[6] = {'a', 'a', '\0', 'b', '\0', 'c'};
124129
// 'b' is behind a null terminator, so should not be found.
125-
ASSERT_STREQ(Func(src, 'b'), nullptr);
130+
ASSERT_TRUE(Func(src, 'b') == nullptr);
126131
// Same goes for 'c'.
127-
ASSERT_STREQ(Func(src, 'c'), nullptr);
132+
ASSERT_TRUE(Func(src, 'c') == nullptr);
128133

129134
// Should find the second of the two a's.
130135
ASSERT_STREQ(Func(src, 'a'), "a");
131136
}
132137

133138
void characterNotWithinStringShouldReturnNullptr() {
134139
// Since 'z' is not within the string, should return nullptr.
135-
ASSERT_STREQ(Func("123?", 'z'), nullptr);
140+
ASSERT_TRUE(Func("123?", 'z') == nullptr);
136141
}
137142

138143
void shouldFindLastOfDuplicates() {
@@ -146,11 +151,12 @@ template <auto Func> struct StrrchrTest : public LIBC_NAMESPACE::testing::Test {
146151

147152
void emptyStringShouldOnlyMatchNullTerminator() {
148153
// Null terminator should match.
149-
ASSERT_STREQ(Func("", '\0'), "");
154+
const char empty_string[] = "";
155+
ASSERT_TRUE(Func(empty_string, '\0') == empty_string);
150156
// All other characters should not match.
151-
ASSERT_STREQ(Func("", 'A'), nullptr);
152-
ASSERT_STREQ(Func("", '2'), nullptr);
153-
ASSERT_STREQ(Func("", '*'), nullptr);
157+
ASSERT_TRUE(Func("", 'A') == nullptr);
158+
ASSERT_TRUE(Func("", '2') == nullptr);
159+
ASSERT_TRUE(Func("", '*') == nullptr);
154160
}
155161
};
156162

0 commit comments

Comments
 (0)