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

Martin Storsjö via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Jan 18 13:35:27 PST 2021


mstorsjo added inline comments.


================
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:
> > 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).
> Well we do that, more or less, but implemented within operations.cpp within an anonymous namespace (so the code calls `detail::stat()` instead of `::stat()`). D91143 takes care of most of what is easily abstractable across platforms.
> 
> For functions that aren't quite as easy to abstract away (where the std::function we're implementing is the abstraction itself) I've gone with ifdefs within the individual functions, see e.g. D91170, where there's already ifdefs with two different codepaths within the function (for varying posix platforms).
Also - I don't mind spending time on trying to factor out that namespace to a separate header for such abstractions, or something like that - but I would very much prefer to do it after this patch set. Right now the easily abstracted bits are already abstracted within the detail namespace in operations.cpp.

I'm pretty convinced that splitting out such a header and abstracting it isn't entirely trivial and would run yet at least another couple rounds in review before it's ok - and at the current rate, a couple rounds in review is a couple weeks at least, and I really don't want to block the patchset with such a refactor, this close to the LLVM 12 branch (Tuesday next week). (The patch has been up for review since Nov 10.)

This is all internal implementation detail, none of it is visible to users.


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

https://reviews.llvm.org/D91141



More information about the libcxx-commits mailing list