[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#1308809 <https://reviews.llvm.org/D54883#1308809>, @MaskRay wrote:

> > 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.


Sure, but very little maintenance cost for us overall.

> We definitely should take each case by case.

I think it's great to ask "is this the right way to do this" for new code. However, once we pick a way to do something, I prefer to use it consistently unless there's a strong reason to deviate.

> 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.

Except that the code being guarded has nothing to do with `__APPLE__` or the behavior on that platform and everything to do with whether `struct stat` has the expected fields. The current conditional inclusion expression more closely relates to what's being guarded, which is one of the reasons why it's an improvement over the proposed change.

>> 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.

We have made passes over this file in the past and removed macros that were no longer required. The last time we did this was back in April.

> 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 same is true for the feature testing macros. For instance, once we updated our minimum system requirements such that stdint.h was provided everywhere, we removed the corresponding feature test macro.

>   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.

"It does not buy us anything" -- yes it does; it buys us exactly what we were sold: does the target architecture support this feature we're relying on? "There's good support for X everywhere *except for*" is exactly why I prefer the feature testing macro -- it means we don't have to guess where the feature is supported and it means that we can trivially identify WHY something is conditionally included rather than wondering why the platform is suddenly important. In this same file, check out `getMainExecutable()` for an example of what this kind of "look at the platform, not the functionality" turns into. I believe:

  #elif defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) ||   \
      defined(__minix) || defined(__DragonFly__) ||                              \
      defined(__FreeBSD_kernel__) || defined(_AIX)

should be spelled `#elif defined(HAVE_GETPROGPATH)`; saving the platform identification checks for times when the behavior of the same API differs between platforms.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54883