[libcxx-commits] [PATCH] D91141: [9/N] [libcxx] Implement the stat function family on top of native windows APIs

Wed Jan 13 08:31:48 PST 2021

mstorsjo added inline comments.

================
Comment at: libcxx/src/filesystem/filesystem_common.h:250-282
+#undef _S_IFMT
+#undef _S_IFDIR
+#undef _S_IFCHR
+#undef _S_IFIFO
+#undef _S_IFREG
+#undef _S_IFBLK
+#undef _S_IFLNK
----------------
ldionne wrote:
> Why are those defines necessary?
Various versions of C runtimes used have fewer or more of them defined: to make things consistent across them, I undefine all existing defines and use our own.

================
Comment at: libcxx/src/filesystem/operations.cpp:402
+// (1601) to the Unix epoch (1970).
+#define FILE_TIME_OFFSET_SECS (uint64_t(369 * 365 + 89) * (24 * 60 * 60))
+
----------------
ldionne wrote:
> Since the approach here is to make Windows into a POSIX-y system by adding `stat` & friends, would it be possible to hide this complexity in some internal header like `windows_posix_shims.h` (or whatever the name)?
Possibly, but there's a couple gotchas:

1. Windows C runtimes actually do have a stat() function, but it's a bit limited (it only provides second precision for times, it doesn't expose the permissions, it doesn't provide something similar to st_dev), so we make our own available in the `detail` namespace here.

2. Not all of these are exact replacements of posix functions. For symlinks (in D91143) I split the `symlink` function into `symlink_file` and `symlink_dir`.

3. Like 2. above, we might want to tweak the interface of the functions outside of the posix interface. E.g. for the stat() function, it's currently implemented by a handful of API calls, to fill in all used fields. If a caller only wants some fields, we could add a flag parameter to `detail::stat` here, indicating exactly which fields we need populated, as a later optimization.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91141