EricWF added inline comments.
================
Comment at: src/experimental/filesystem/operations.cpp:23-28
+// We can use the presence of UTIME_OMIT to detect platforms that do not
+// provide utimensat, with some exceptions on OS X.
+#if !defined(UTIME_OMIT) || \
+ (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) &&
__MAC_OS_X_VERSION_MIN_REQUIRED < 1030)
+#define _LIBCPP_HAS_NO_UTIMENSAT
+#endif
----------------
dexonsmith wrote:
> dexonsmith wrote:
> > EricWF wrote:
> > > dexonsmith wrote:
> > > > Sadly this isn't quite sufficient. As per Jack's suggested SDK patch
> > > > in the PR, we need to enumerate the platforms :/. I think this should
> > > > be the right logic for the four Darwin platforms:
> > > >
> > > > (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) &&
> > > > __IPHONE_OS_VERSION_MIN_REQUIRED < __IPHONE_11_0) || \
> > > > (defined(__WATCH_OS_VERSION_MIN_REQUIRED) &&
> > > > __WATCH_OS_VERSION_MIN_REQUIRED < __WATCHOS_4_0) || \
> > > > (defined(__TV_OS_VERSION_MIN_REQUIRED) &&
> > > > __TV_OS_VERSION_MIN_REQUIRED < __TVOS_11_0) || \
> > > > (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) &&
> > > > __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_13)
> > > >
> > > Do we have to do the below dance for all of those macros?
> > >
> > > ```
> > > #if !defined(__FOO_VERSION_MIN_REQUIRED) &&
> > > defined(__ENVIROMENT_FOO_VERSION_MIN_REQUIRED)
> > > #define __FOO_VERSION_MIN_REQUIRED __ENVIROMENT_FOO_VERSION_REQUIRED
> > > #endif
> > > ```
> > Nope. I just advised you to use the wrong ones. Use the `__ENVIRONMENT`
> > versions pre-defined by Clang.
> Specifically:
>
> ```
> clang/lib/Basic/Targets.cpp:183:
> Builder.defineMacro("__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__", Str);
> clang/lib/Basic/Targets.cpp:185:
> Builder.defineMacro("__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__",
> clang/lib/Basic/Targets.cpp:197:
> Builder.defineMacro("__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__", Str);
> clang/lib/Basic/Targets.cpp:221:
> Builder.defineMacro("__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__", Str);
> ```
Ack, I'll switch to using those.
================
Comment at: src/experimental/filesystem/operations.cpp:22-24
+#if defined(__APPLE__)
+#include <Availability.h>
+#endif
----------------
dexonsmith wrote:
> EricWF wrote:
> > dexonsmith wrote:
> > > I only just noticed you were including Availability.h. That shouldn't be
> > > necessary, since the macros should be defined by the compiler.
> > __MAC_10_13 et al are defined in `Availability.h`, and
> > `AvailabilityInternal.h` seems to do the `__ENV` dance described above. Are
> > you sure it's not needed?
> I don't think the dance is necessary, since libcxx won't be overriding those
> macros.
>
> Also, we can skip the `__MAC_10_13` macros, ala src/chrono.cpp.
Using the `__MAC_10_13` seems preferable, and since this is in a source file,
and not a header, we lose nothing by including `<Availability.h>`.
https://reviews.llvm.org/D34249
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
