[libc-commits] [PATCH] D147822: [libc] Add memmem implementation

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet accepted this revision.
gchatelet added a comment.

Sorry for the lag, Monday was off in France and I missed the patch today.
I'm not blocking this patch but please consider the suggested changes.



================
Comment at: libc/src/string/memory_utils/memmem_implementations.h:18
+constexpr static void *
+memmem_implementation(const void *haystack, size_t haystack_len,
+                      const void *needle, size_t needle_len, Comp &&comp) {
----------------
Not necessarily for this patch but it may be better to use the `cpp::string_view` abstraction for both needle and haystack.

We should also return a `const char*` to keep types as sane as possible.
This way we `const_cast` only where it actually matters, i.e. in `memmem`.



================
Comment at: libc/test/src/string/memmem_test.cpp:81
+  {
+    char h[] = {
+        'a',
----------------
put on a single line


================
Comment at: libc/test/src/string/memmem_test.cpp:97
+    char h[] = {'\0'};
+    char n[] = {
+        '\0',
----------------
same here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147822/new/

https://reviews.llvm.org/D147822