Skip to content

Conversation

@enh-google
Copy link
Contributor

Three implementations of wcschr() is two too many.

@llvmbot llvmbot added the libc label Jul 25, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 25, 2025

@llvm/pr-subscribers-libc

Author: None (enh-google)

Changes

Three implementations of wcschr() is two too many.


Full diff: https://github.com/llvm/llvm-project/pull/150661.diff

4 Files Affected:

  • (modified) libc/src/wchar/wchar_utils.h (+4-7)
  • (modified) libc/src/wchar/wcschr.cpp (+2-5)
  • (modified) libc/src/wchar/wcspbrk.cpp (+1-9)
  • (modified) libc/src/wchar/wcstok.cpp (+3-10)
diff --git a/libc/src/wchar/wchar_utils.h b/libc/src/wchar/wchar_utils.h
index e0218c7d89b1f..d69638fa71912 100644
--- a/libc/src/wchar/wchar_utils.h
+++ b/libc/src/wchar/wchar_utils.h
@@ -17,13 +17,10 @@
 namespace LIBC_NAMESPACE_DECL {
 namespace internal {
 
-// returns true if the character exists in the string
-LIBC_INLINE static bool wcschr(wchar_t c, const wchar_t *str) {
-  for (int n = 0; str[n]; ++n) {
-    if (str[n] == c)
-      return true;
-  }
-  return false;
+LIBC_INLINE static wchar_t *wcschr(const wchar_t *s, wchar_t c) {
+  for (; *s && *s != c; ++s)
+    ;
+  return (*s == c) ? s : nullptr;
 }
 
 // bool should be true for wcscspn for complimentary span
diff --git a/libc/src/wchar/wcschr.cpp b/libc/src/wchar/wcschr.cpp
index defc2ce3c3b72..e53ec9a4c95ba 100644
--- a/libc/src/wchar/wcschr.cpp
+++ b/libc/src/wchar/wcschr.cpp
@@ -11,15 +11,12 @@
 #include "hdr/types/wchar_t.h"
 #include "src/__support/common.h"
 #include "src/__support/macros/config.h"
+#include "wchar_utils.h"
 
 namespace LIBC_NAMESPACE_DECL {
 
 LLVM_LIBC_FUNCTION(const wchar_t *, wcschr, (const wchar_t *s, wchar_t c)) {
-  for (; *s && *s != c; ++s)
-    ;
-  if (*s == c)
-    return s;
-  return nullptr;
+  return internal::wcschr(s, c);
 }
 
 } // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/wchar/wcspbrk.cpp b/libc/src/wchar/wcspbrk.cpp
index a00ba9979a489..9faa34fe5fd1a 100644
--- a/libc/src/wchar/wcspbrk.cpp
+++ b/libc/src/wchar/wcspbrk.cpp
@@ -14,14 +14,6 @@
 
 namespace LIBC_NAMESPACE_DECL {
 
-bool contains_char(const wchar_t *str, wchar_t target) {
-  for (; *str != L'\0'; str++)
-    if (*str == target)
-      return true;
-
-  return false;
-}
-
 LLVM_LIBC_FUNCTION(const wchar_t *, wcspbrk,
                    (const wchar_t *src, const wchar_t *breakset)) {
   LIBC_CRASH_ON_NULLPTR(src);
@@ -29,7 +21,7 @@ LLVM_LIBC_FUNCTION(const wchar_t *, wcspbrk,
 
   // currently O(n * m), can be further optimized to O(n + m) with a hash set
   for (int src_idx = 0; src[src_idx] != 0; src_idx++)
-    if (contains_char(breakset, src[src_idx]))
+    if (internal::wcschr(breakset, src[src_idx]))
       return src + src_idx;
 
   return nullptr;
diff --git a/libc/src/wchar/wcstok.cpp b/libc/src/wchar/wcstok.cpp
index 291efc15e158a..32a500bf302c7 100644
--- a/libc/src/wchar/wcstok.cpp
+++ b/libc/src/wchar/wcstok.cpp
@@ -13,15 +13,8 @@
 
 namespace LIBC_NAMESPACE_DECL {
 
-bool isADelimeter(wchar_t wc, const wchar_t *delimiters) {
-  for (const wchar_t *delim_ptr = delimiters; *delim_ptr != L'\0'; ++delim_ptr)
-    if (wc == *delim_ptr)
-      return true;
-  return false;
-}
-
 LLVM_LIBC_FUNCTION(wchar_t *, wcstok,
-                   (wchar_t *__restrict str, const wchar_t *__restrict delim,
+                   (wchar_t *__restrict str, const wchar_t *__restrict delims,
                     wchar_t **__restrict context)) {
   if (str == nullptr) {
     if (*context == nullptr)
@@ -31,11 +24,11 @@ LLVM_LIBC_FUNCTION(wchar_t *, wcstok,
   }
 
   wchar_t *tok_start, *tok_end;
-  for (tok_start = str; *tok_start != L'\0' && isADelimeter(*tok_start, delim);
+  for (tok_start = str; *tok_start != L'\0' && wcschr(delims, *tok_start);
        ++tok_start)
     ;
 
-  for (tok_end = tok_start; *tok_end != L'\0' && !isADelimeter(*tok_end, delim);
+  for (tok_end = tok_start; *tok_end != L'\0' && !wcschr(delims, *tok_end);
        ++tok_end)
     ;
 

@lntue
Copy link
Contributor

lntue commented Jul 25, 2025

errors from the pre-commit:

In file included from /home/runner/work/llvm-project/llvm-project/libc/src/wchar/wcstok.cpp:13:
/home/runner/work/llvm-project/llvm-project/libc/src/wchar/wchar_utils.h:23:10: error: cannot initialize return object of type 'wchar_t *' with an rvalue of type 'const wchar_t *'
   23 |   return (*s == c) ? s : nullptr;
      |          ^~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/llvm-project/llvm-project/libc/src/wchar/wchar_utils.h:32:19: error: no matching function for call to 'wcschr'
   32 |     bool in_set = wcschr(s1[i], s2);
      |                   ^~~~~~
/home/runner/work/llvm-project/llvm-project/libc/src/wchar/wchar_utils.h:20:29: note: candidate function not viable: no known conversion from 'const wchar_t' to 'const wchar_t *' for 1st argument; take the address of the argument with &
   20 | LIBC_INLINE static wchar_t *wcschr(const wchar_t *s, wchar_t c) {
      |                             ^      ~~~~~~~~~~~~~~~~
   21 |   for (; *s && *s != c; ++s)
   22 |     ;
   23 |   return (*s == c) ? s : nullptr;
   24 | }

The exported function is using a similar white lie for convenience, so the internal function may as well rather than adding const casts.
Copy link
Contributor

@uzairnawaz uzairnawaz left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes, I didn't realize how often we were rewriting the same code 😅

@github-actions
Copy link

github-actions bot commented Jul 25, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@frobtech frobtech left a comment

Choose a reason for hiding this comment

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

lgtm. We need a clang-tidy checker to reject static used in headers.

@enh-google enh-google merged commit 701de35 into llvm:main Jul 28, 2025
14 of 19 checks passed
ingomueller-net added a commit to ingomueller-net/llvm-project that referenced this pull request Jul 29, 2025
This was broken by llvm#150661, which added includes to `wcspbrk.cpp` and
`wcschr.cpp` that weren't in the dependencies of the corresponding
targets.

Signed-off-by: Ingo Müller <[email protected]>
lntue pushed a commit that referenced this pull request Jul 29, 2025
This was broken by #150661, which added includes to `wcspbrk.cpp` and
`wcschr.cpp` that weren't in the dependencies of the corresponding
targets.

Signed-off-by: Ingo Müller <[email protected]>
ingomueller-net added a commit that referenced this pull request Jul 29, 2025
This adds two dependencies that I wanted to be part of #151090 but
forgot to add them before the last push, which related to `wcschr.cpp`
and were originally broken by #150661.

Signed-off-by: Ingo Müller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants