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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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
----------------
mstorsjo wrote:
> mstorsjo wrote:
> > 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.
> I guess such a clarifying comment would help here.
Just to understand, you're talking about the runtime headers for various versions of Windows? Is this not possible to fix by requiring a recent-ish version of those headers when building libc++?


================
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))
+
----------------
mstorsjo wrote:
> 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.
> 
I guess my actual ask is to properly define *some* cross-platform API that we call from this file. If it's not exactly POSIX, that's fine, however that should be documented.

We're porting the library to a new platform, so my preference would be that we delineate the minimal API that needs to be implemented by the system for the rest of the filesystem library to work. I'm not attached to whether we do it in this patch or later, but I think we need to do it somewhere in this patch list (I haven't looked at all of them, so if you do that later on, please LMK).


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

https://reviews.llvm.org/D91141