[PATCH] D54883: [Support/FileSystem] Use st_atimespec only when __APPLE__ is defined

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a comment.

In D54883#1308313 <https://reviews.llvm.org/D54883#1308313>, @MaskRay wrote:

> In D54883#1307857 <https://reviews.llvm.org/D54883#1307857>, @aaron.ballman wrote:
>
> > In D54883#1307853 <https://reviews.llvm.org/D54883#1307853>, @krytarowski wrote:
> >
> > > In D54883#1307787 <https://reviews.llvm.org/D54883#1307787>, @aaron.ballman wrote:
> > >
> > > > Do our minimum system requirements for building LLVM include requiring POSIX 2008 conformance (perhaps indirectly, through the minimum toolchain requirements)?
> > >
> > >
> > > There are almost no OSes having conformance with UNIX/POSIX. UNIXv6 (or similar) is Solaris only.
> >
> >
> > If our existing system requirements aren't sufficient to practically guarantee these fields are available, I'd prefer to keep the autodetect code and rely on that instead.
>
>
> There are 2 pieces of autodetect code:
>
> 1. preprocessor level detection `#if defined(HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC)`
> 2. cmake detection `CHECK_STRUCT_HAS_MEMBER`
>
>   After this patch, they get simplified and become:
> 3. preprocessor level detection `#ifdef __APPLE__`


I recognize that, but I don't think it's all that much more simple (we still have to test for `__APPLE__`) and worry that it is also more fragile because it removes the dynamic cmake detection.

> On https://github.com/apple/darwin-xnu/blob/master/bsd/sys/stat.h , I cannot find a better feature macro to test so I have to use `__APPLE__`. When they start to support `st_atim` (which is widely available on other platforms) in a newer version, we can make the `#ifdef __APPLE__` check more specific.
> 
> The patch directly tests for the necessary prerequisite and removes two cmake variables. This makes it substantially more build system agnostic (for people who use gn,bazel,... they don't need to re-invent the feature detection mechanism in their build systems) and probably (ironically) more portable.

Given that we already use feature detection rather heavily, I don't think deviating here buys us all that much in practice.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54883