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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

We can land this after my comments from this round are addressed.



================
Comment at: libc/include/llvm-libc-types/ino_t.h:12
 
-typedef __UINTPTR_TYPE__ ino_t;
+typedef __UINT64_TYPE__ ino_t;
 
----------------
Is this required for this change? If not, remove it. If yes, can explain why?

Defining `ino_t` to be a typedef of `__UINTPTR_TYPE__` might not be the best but it was done because the size of `ino_t` matches the pointer size on most platforms. If have ways to do this better, feel free to send a separate patch.


================
Comment at: libc/include/llvm-libc-types/off64_t.h:12
-
-typedef __INT64_TYPE__ off64_t;
-
----------------
mikhail.ramalho wrote:
> michaelrj wrote:
> > I didn't mean you should delete `off64_t` completely, only that you didn't need it in that specific place.
> We don't actually need it since, as you mentioned, off_t and off64_t are the same types (the same for ino_t and ino64_t). Do we want to keep them both in the code base?
Linux man pages for fopencookie explicitly specify `off64_t` so we should keep it.


================
Comment at: libc/src/__support/File/linux_dir.cpp:45
+                                        buffer.size());
+#elif defined(SYS_getdents)
+  size_t size =
----------------
Do we need this alternate path at all for the platforms we care about now? For example, if none of the builders exercise this path, then it is dead code. The four platforms I have checked and care for at this time, x86_64, aarch64, arm32 and riscv64, all have `SYS_getdents64`. If this alternate path is not an immediate necessity, we should skip it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147738