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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/include/llvm-libc-types/struct_linux_dirent.h:1
+//===-- Definition of type struct linux dirent ----------------------------===//
+//
----------------
michaelrj wrote:
> mikhail.ramalho wrote:
> > sivachandra wrote:
> > > This is an internal type so should not be define in a public header file.
> > This sounds contradictory to what @michaelrj suggested in one of his comments:
> > 
> > `glibc not providing the struct publicly doesn't mean that we can't. I think it's cleaner to have the type defined in llvm-libc-types and then use #ifdef to include the appropriate struct.`
> From what I can tell from the man page, the `linux_dirent` struct is somewhat standard, but also often not defined by the C library. Given that, I personally prefer defining it like the other `dirent` struct as a public type. If we decide not to define it publicly, I think we should still define it in a separate header and use `ifdef` to include it as needed. That header might end up in `__support/File/linux`.
Sorry, I missed @michaelrj's comment - AFAICT, `linux_dirent` is only relevant for the syscall, so we should not expose it publicly. We have examples of other such types in the libc: https://github.com/llvm/llvm-project/blob/main/libc/src/signal/linux/signal_utils.h#L29. And since it would be an internal type, you should name it `LinuxDirent`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147738