[libc-commits] [PATCH] D147738: [libc] Enable linux directory entries syscalls in riscv64

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/include/dirent.h.def:14
 
+#ifdef __unix__
+#include <llvm-libc-types/struct_linux_dirent.h>
----------------
if this is a linux type then it probably makes more sense for this to be `__linux__`


================
Comment at: libc/include/llvm-libc-types/off64_t.h:12
-
-typedef __INT64_TYPE__ off64_t;
-
----------------
I didn't mean you should delete `off64_t` completely, only that you didn't need it in that specific place.


================
Comment at: libc/src/__support/File/dir.cpp:16-17
 
 #include <stdlib.h>
+#include <string.h> // For strlen
 
----------------
now that we've moved to `cpp/new.h` for allocation, you should be able to remove the `stdlib` include as well as the `string` include.


================
Comment at: libc/src/__support/File/dir.cpp:52
+  size_t struct_size =
+      sizeof(ino_t) + sizeof(unsigned char) + strlen(ld->d_name);
+#ifdef __unix__
----------------
We generally avoid calling public names of libc function in our internal code, if you want to call strlen you should use the internal implementation in `string/string_utils.h` called `string_length`.


================
Comment at: libc/src/__support/File/dir.cpp:70
+  size_t d_name_offset = offsetof(linux_dirent, d_name);
+  memmove(buffer + readptr + d_name_offset + 1,
+          buffer + readptr + d_name_offset, strlen(ld->d_name));
----------------
You should also use the internal `memmove` which is in `src/string/memory_utils/memmove_implementations.h` and `memset` (same pattern)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147738