[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

I went ahead and landed this with the comments requested by Duncan in r324385. 
(http://llvm.org/viewvc/llvm-project?view=revision=324385)

I'm also quoting Duncan's email reply to my comment, since that's not showing 
up on phab:
"""
Using runtime availability checking doesn't make sense for a system Libc++, as 
you point out.  If we add runtime checks they ought to be non-default, and 
hidden behind configuration flags.

Also, do I remember correctly that __builtin_available requires linking against 
Foundation (and thus the Obj-C runtime) for NSProcessInfo, or has that 
dependency been removed/avoided?  It would be surprising for the C++ standard 
library to pull in the Objective-C runtime.  A reasonable configuration option, 
but not a reasonable default behaviour IMO.
"""


https://reviews.llvm.org/D34249



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Nico Weber via cfe-commits]

(To be clear, the patch as-is is 100% fine with me.)

On Thu, Feb 1, 2018 at 11:44 AM, Duncan P. N. Exon Smith via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Using runtime availability checking doesn't make sense for a system
> Libc++, as you point out.  If we add runtime checks they ought to be
> non-default, and hidden behind configuration flags.
>
> Also, do I remember correctly that __builtin_available requires linking
> against Foundation (and thus the Obj-C runtime) for NSProcessInfo, or has
> that dependency been removed/avoided?  It would be surprising for the C++
> standard library to pull in the Objective-C runtime.  A reasonable
> configuration option, but not a reasonable default behaviour IMO.
>
> > On Feb 1, 2018, at 05:52, Nico Weber via Phabricator <
> revi...@reviews.llvm.org> wrote:
> >
> > thakis added a comment.
> >
> > The powers that be updated the SDK on the chromium clang bots, so we
> need some solution for this issue soon.
> >
> > The approach in this patch should work, but it seems conservative. Now
> libc++ only uses utimesat() if it's built with a deployment target of macOS
> 10.13. But if we were to set a deployment target of 10.12 and then _run_ on
> 10.13, we could use utimesat() as well, even though this patch currently
> doesn't. (Apple bundles libc++ with the system, so it's not a meaningful
> difference for Apple-built libc++'s, but applications can build libc++
> themselves and statically link to it, and for those it would make a
> difference.)
> >
> > https://clang.llvm.org/docs/LanguageExtensions.html#
> objective-c-available has some words on it (except that `@available` is
> spelled `__builtin_available` in non-Objective-C code) -- thoughts on using
> runtime detection of utimes() instead? That's how things are supposed to
> work on the Apple platforms.
> >
> >
> > https://reviews.llvm.org/D34249
> >
> >
> >
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Duncan P. N. Exon Smith via cfe-commits]

Using runtime availability checking doesn't make sense for a system Libc++, as 
you point out.  If we add runtime checks they ought to be non-default, and 
hidden behind configuration flags.

Also, do I remember correctly that __builtin_available requires linking against 
Foundation (and thus the Obj-C runtime) for NSProcessInfo, or has that 
dependency been removed/avoided?  It would be surprising for the C++ standard 
library to pull in the Objective-C runtime.  A reasonable configuration option, 
but not a reasonable default behaviour IMO.

> On Feb 1, 2018, at 05:52, Nico Weber via Phabricator 
>  wrote:
> 
> thakis added a comment.
> 
> The powers that be updated the SDK on the chromium clang bots, so we need 
> some solution for this issue soon.
> 
> The approach in this patch should work, but it seems conservative. Now libc++ 
> only uses utimesat() if it's built with a deployment target of macOS 10.13. 
> But if we were to set a deployment target of 10.12 and then _run_ on 10.13, 
> we could use utimesat() as well, even though this patch currently doesn't. 
> (Apple bundles libc++ with the system, so it's not a meaningful difference 
> for Apple-built libc++'s, but applications can build libc++ themselves and 
> statically link to it, and for those it would make a difference.)
> 
> https://clang.llvm.org/docs/LanguageExtensions.html#objective-c-available has 
> some words on it (except that `@available` is spelled `__builtin_available` 
> in non-Objective-C code) -- thoughts on using runtime detection of utimes() 
> instead? That's how things are supposed to work on the Apple platforms.
> 
> 
> https://reviews.llvm.org/D34249
> 
> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

The powers that be updated the SDK on the chromium clang bots, so we need some 
solution for this issue soon.

The approach in this patch should work, but it seems conservative. Now libc++ 
only uses utimesat() if it's built with a deployment target of macOS 10.13. But 
if we were to set a deployment target of 10.12 and then _run_ on 10.13, we 
could use utimesat() as well, even though this patch currently doesn't. (Apple 
bundles libc++ with the system, so it's not a meaningful difference for 
Apple-built libc++'s, but applications can build libc++ themselves and 
statically link to it, and for those it would make a difference.)

https://clang.llvm.org/docs/LanguageExtensions.html#objective-c-available has 
some words on it (except that `@available` is spelled `__builtin_available` in 
non-Objective-C code) -- thoughts on using runtime detection of utimes() 
instead? That's how things are supposed to work on the Apple platforms.


https://reviews.llvm.org/D34249



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added inline comments.



Comment at: src/experimental/filesystem/operations.cpp:27
+// 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) ||
\

Would it be easier to read if we split this test into two halves - one for 
Apple platforms and the other for the rest of the world?

#if !defined(__APPLE__)
# if !defined(UTIME_OMIT)
# define _LIBCPP_HAS_NO_UTIMENSAT
# endif
#else
.
#endif



https://reviews.llvm.org/D34249



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Eric Fiselier via Phabricator via cfe-commits]

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 
+#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 ``. 



https://reviews.llvm.org/D34249



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: src/experimental/filesystem/operations.cpp:22-24
+#if defined(__APPLE__)
+#include 
+#endif

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.


https://reviews.llvm.org/D34249



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: src/experimental/filesystem/operations.cpp:22-24
+#if defined(__APPLE__)
+#include 
+#endif

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?


https://reviews.llvm.org/D34249



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith 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:
> 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);
```


https://reviews.llvm.org/D34249



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith 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

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.



Comment at: src/experimental/filesystem/operations.cpp:22-24
+#if defined(__APPLE__)
+#include 
+#endif

I only just noticed you were including Availability.h.  That shouldn't be 
necessary, since the macros should be defined by the compiler.


https://reviews.llvm.org/D34249



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Eric Fiselier via Phabricator via cfe-commits]

EricWF updated this revision to Diff 102783.
EricWF marked an inline comment as done.
EricWF added a comment.

- Address inline comments.


https://reviews.llvm.org/D34249

Files:
  src/experimental/filesystem/operations.cpp


Index: src/experimental/filesystem/operations.cpp
===
--- src/experimental/filesystem/operations.cpp
+++ src/experimental/filesystem/operations.cpp
@@ -19,7 +19,25 @@
 #include 
 #include 
 #include   /* values for fchmodat */
-#if !defined(UTIME_OMIT)
+#if defined(__APPLE__)
+#include 
+#endif
+
+// 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(__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)
+#define _LIBCPP_HAS_NO_UTIMENSAT
+#endif
+
+#if defined(_LIBCPP_HAS_NO_UTIMENSAT)
 #include  // for ::utimes as used in __last_write_time
 #endif
 
@@ -682,9 +700,7 @@
 using namespace std::chrono;
 std::error_code m_ec;
 
-// We can use the presence of UTIME_OMIT to detect platforms that do not
-// provide utimensat.
-#if !defined(UTIME_OMIT)
+#if defined(_LIBCPP_HAS_NO_UTIMENSAT)
 // This implementation has a race condition between determining the
 // last access time and attempting to set it to the same value using
 // ::utimes


Index: src/experimental/filesystem/operations.cpp
===
--- src/experimental/filesystem/operations.cpp
+++ src/experimental/filesystem/operations.cpp
@@ -19,7 +19,25 @@
 #include 
 #include 
 #include   /* values for fchmodat */
-#if !defined(UTIME_OMIT)
+#if defined(__APPLE__)
+#include 
+#endif
+
+// 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(__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)
+#define _LIBCPP_HAS_NO_UTIMENSAT
+#endif
+
+#if defined(_LIBCPP_HAS_NO_UTIMENSAT)
 #include  // for ::utimes as used in __last_write_time
 #endif
 
@@ -682,9 +700,7 @@
 using namespace std::chrono;
 std::error_code m_ec;
 
-// We can use the presence of UTIME_OMIT to detect platforms that do not
-// provide utimensat.
-#if !defined(UTIME_OMIT)
+#if defined(_LIBCPP_HAS_NO_UTIMENSAT)
 // This implementation has a race condition between determining the
 // last access time and attempting to set it to the same value using
 // ::utimes
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Eric Fiselier via Phabricator via cfe-commits]

EricWF updated this revision to Diff 102782.

https://reviews.llvm.org/D34249

Files:
  src/experimental/filesystem/operations.cpp


Index: src/experimental/filesystem/operations.cpp
===
--- src/experimental/filesystem/operations.cpp
+++ src/experimental/filesystem/operations.cpp
@@ -18,9 +18,25 @@
 #include 
 #include 
 #include 
+#include  /* for ::utimes as used in __last_write_time */
 #include   /* values for fchmodat */
-#if !defined(UTIME_OMIT)
-#include  // for ::utimes as used in __last_write_time
+
+#if defined(__APPLE__)
+#include 
+#endif
+
+// 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(__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)
+#define _LIBCPP_HAS_NO_UTIMENSAT
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_FILESYSTEM
@@ -682,9 +698,7 @@
 using namespace std::chrono;
 std::error_code m_ec;
 
-// We can use the presence of UTIME_OMIT to detect platforms that do not
-// provide utimensat.
-#if !defined(UTIME_OMIT)
+#if defined(_LIBCPP_HAS_NO_UTIMENSAT)
 // This implementation has a race condition between determining the
 // last access time and attempting to set it to the same value using
 // ::utimes


Index: src/experimental/filesystem/operations.cpp
===
--- src/experimental/filesystem/operations.cpp
+++ src/experimental/filesystem/operations.cpp
@@ -18,9 +18,25 @@
 #include 
 #include 
 #include 
+#include  /* for ::utimes as used in __last_write_time */
 #include   /* values for fchmodat */
-#if !defined(UTIME_OMIT)
-#include  // for ::utimes as used in __last_write_time
+
+#if defined(__APPLE__)
+#include 
+#endif
+
+// 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(__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)
+#define _LIBCPP_HAS_NO_UTIMENSAT
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_FILESYSTEM
@@ -682,9 +698,7 @@
 using namespace std::chrono;
 std::error_code m_ec;
 
-// We can use the presence of UTIME_OMIT to detect platforms that do not
-// provide utimensat.
-#if !defined(UTIME_OMIT)
+#if defined(_LIBCPP_HAS_NO_UTIMENSAT)
 // This implementation has a race condition between determining the
 // last access time and attempting to set it to the same value using
 // ::utimes
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Eric Fiselier via Phabricator via cfe-commits]

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:
> 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
```


https://reviews.llvm.org/D34249



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

This is the right idea, although it only covers macOS.

Any ideas for how to test this?




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

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)



https://reviews.llvm.org/D34249



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Eric Fiselier via Phabricator via cfe-commits]

EricWF created this revision.

This fixes llvm.org/PR33469.


https://reviews.llvm.org/D34249

Files:
  src/experimental/filesystem/operations.cpp


Index: src/experimental/filesystem/operations.cpp
===
--- src/experimental/filesystem/operations.cpp
+++ src/experimental/filesystem/operations.cpp
@@ -19,7 +19,15 @@
 #include 
 #include 
 #include   /* values for fchmodat */
-#if !defined(UTIME_OMIT)
+
+// 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
+
+#if defined(_LIBCPP_HAS_NO_UTIMENSAT)
 #include  // for ::utimes as used in __last_write_time
 #endif
 
@@ -682,9 +690,7 @@
 using namespace std::chrono;
 std::error_code m_ec;
 
-// We can use the presence of UTIME_OMIT to detect platforms that do not
-// provide utimensat.
-#if !defined(UTIME_OMIT)
+#if defined(_LIBCPP_HAS_NO_UTIMENSAT)
 // This implementation has a race condition between determining the
 // last access time and attempting to set it to the same value using
 // ::utimes


Index: src/experimental/filesystem/operations.cpp
===
--- src/experimental/filesystem/operations.cpp
+++ src/experimental/filesystem/operations.cpp
@@ -19,7 +19,15 @@
 #include 
 #include 
 #include   /* values for fchmodat */
-#if !defined(UTIME_OMIT)
+
+// 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
+
+#if defined(_LIBCPP_HAS_NO_UTIMENSAT)
 #include  // for ::utimes as used in __last_write_time
 #endif
 
@@ -682,9 +690,7 @@
 using namespace std::chrono;
 std::error_code m_ec;
 
-// We can use the presence of UTIME_OMIT to detect platforms that do not
-// provide utimensat.
-#if !defined(UTIME_OMIT)
+#if defined(_LIBCPP_HAS_NO_UTIMENSAT)
 // This implementation has a race condition between determining the
 // last access time and attempting to set it to the same value using
 // ::utimes
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits