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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

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

We do have lots of feature detection macros in use but each brings some maintenance cost and we should be cautious to introduce new ones. We definitely should take each case by case. For this particular case, we know that `st_atim` has good support for systems other than macOS.
If I have to write `defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__)`, I'll start considering whether a `HAVE_*` macro can do a better job. But here, a simple check like `#ifdef __APPLE__` suffices.

> The CMake feature is working correctly for all supported use cases, and buys us much more portability than the replacement. I'm yet to hear any actual advantage from not using CMake check.

cmake variables and `HAVE_*` macros are easy to introduce but hard to remove. For this `st_atim` case, `__APPLE__` is lacking behind but we can envision it will catch up later, and we can eventually remove the check.  The check is also intentionally dichotomous instead of trichotomous as `atime_nsec = mtime_nsec = 0;` shouldn't happen. Readers will not stop here and wonder what happens if neither macros is defined.
Portability is considered with the universal set in mind. It does not buy us anything to extend our support unlimited to some unknown hypothetical platforms. If such platform does exist and this patch breaks it, I hope its users will stop for 1 second and think why their POSIX platform still hasn't be prepared for a standard released 10 years ago. On the other hand, the code is evolving. Though for portability it might not happen, if `__APPLE__` removed `st_atimespec` or the hypothetical platform's users want to fix the code, I believe the case is simple enough for contributors to fix.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54883