[PATCH] D97410: [llvm] Check availability for os_signpost

[Daniel Sanders via Phabricator via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM with a couple nitpicks



================
Comment at: llvm/lib/Support/Signposts.cpp:52
+    os_signpost_id_t ID = {};
+    if (__builtin_available(macos 10.14, iOS 12, tvOS 12, watchOS 5, *)) {
+      ID = os_signpost_id_make_with_pointer(getLogger(), O);
----------------
We might want to wrap the version list in a macro so we don't have to dupe it in each function. I'm thinking something along the lines of:
```
#define SIGNPOSTS_AVAILABLE() __builtin_available(macos 10.14, iOS 12, tvOS 12, watchOS 5, *)
```


================
Comment at: llvm/lib/Support/Signposts.cpp:55
+    }
+    const auto &Inserted = Signposts.insert(std::make_pair(O, ID));
     return Inserted.first->second;
----------------
Do we still need to insert it when signposts aren't available? The call to getSignpostForObject() isn't reachable in that case


================
Comment at: llvm/lib/Support/Signposts.cpp:70
     if (isEnabled()) {
-      // Both strings used here are required to be constant literal strings.
-      os_signpost_interval_begin(getLogger(), getSignpostForObject(O),
-                                 "LLVM Timers", "Begin %s", Name.data());
+      if (__builtin_available(macos 10.14, iOS 12, tvOS 12, watchOS 5, *)) {
+        // Both strings used here are required to be constant literal strings.
----------------
Is this needed? isEnabled() will be false but I can believe the warning isn't smart enough to see that.


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

https://reviews.llvm.org/D97410