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

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

I don't really have an opinion on this patch, but I'd like to mention a few things.

- The feature test macros generally do have two downsides:
  1. They can have false positives. We had one bug where the feature detection for mmap() was accidentally true on Windows which lead to pretty bad behavior.
  2. They slow down cmake. Every single check probably not a ton, but running cmake on my system takes minutes, much of this time doing feature checks that return the same every time.
- maskray somewhere says that this helps the gn build, but a) the gn build explicitly shouldn't be an argument for making changes in the cmake build, see llvm/utils/gn/README.rst, so please don't do that b) this was very easy to merge to the gn build (see r347530).

I personally think doing something like

  if (APPLE)
    (set HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC 1)
    (set HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC 0)
  else()
    (set HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC 1)
    (set HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC 0)
  endif()

if that's what the check computes on every system makes a lot of sense, but it's in the end a philosophical decision on if you want your systems to be fast, predictable, and inflexible, or slow, unpredictable, and flexible.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54883