[PATCH] D107970: [LLVM][Support] Add macOS 12 to Triple.cpp

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Support/Triple.cpp:1767
-
-  llvm_unreachable("invalid arch name");
 }
----------------
dexonsmith wrote:
> jrtc27 wrote:
> > Don't include unrelated changes
> Why remove this?
I assume because a sufficiently-smart compiler can notice it's dead code since the above switch is exhaustive and always returns. But a sufficiently-dumb compiler wouldn't notice that and need something to avoid a warning about the opposite...


================
Comment at: llvm/lib/Support/Triple.cpp:1775
       return VersionTuple(11, 0);
+    // macOS 10.17 is canonicalized to macOS 12.
+    // TODO: Find a better way to do this going forward rather than hardcoding a
----------------
dexonsmith wrote:
> jrtc27 wrote:
> > This function isn't used by LLVM, it's only used by Swift, and I don't know if they need to support 10.17 meaning 12 or whether 10.16 meaning 11 was just a one-off due to having a bunch of code already written that was using 10.16 in @available etc annotations. IMO this should be left alone until someone from Apple decides they do in fact need this; their fork of LLVM doesn't currently have this function changed, for example, though that could just be because they haven't made Monterey changes public yet (but if it's needed they'll have already discovered that long ago).
> FWIW, this function is also used by clang downstream (e.g., see https://github.com/apple/llvm-project/blob/next/clang/lib/Sema/SemaDeclAttr.cpp#L2473).
Ah, didn't think to look at your Clang, but in hindsight should probably have realised that the (Obj)C(++) equivalent attributes would likely have the same issue...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107970