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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a reviewer: arphaman.
dexonsmith added a subscriber: arphaman.
dexonsmith added a comment.

I believe this code change isn't needed, but @arphaman can confirm. Adding him as a blocking reviewer since he's been upstreaming other related code.

@gAlfonso-bit, FYI, if you're eager to help out, many of Apple's downstream changes have been officially contributed to the LLVM project at https://github.com/llvm/llvm-project-staging (see, e.g., the staging/apple branch), but are waiting to be separated, likely cleaned up a bit, and then reviewed (here, at http://reviews.llvm.org). Reach out to me over email if you're interested in tackling something. I can help choose what to break off and/or help find a reviewer.



================
Comment at: llvm/lib/Support/Triple.cpp:1143
     // Ignore the version from the triple.  This is only handled because the
-    // the clang driver combines OS X and IOS support into a common Darwin
+    // the clang driver combines OS X and iOS support into a common Darwin
     // toolchain that wants to know the OS X version number even when targeting
----------------
jrtc27 wrote:
> You're not touching these functions, leave them alone
BTW, I'd be happy to review a standalone, NFC patch that fixes these comments (IOS vs iOS). (I agree with @jrtc27 that it's better not to include unrelated changes in this patch.)


================
Comment at: llvm/lib/Support/Triple.cpp:1637
 
-  // If vendor is apple, ignore the version number.
+  // If vendor is Apple, ignore the version number.
   if (getVendor() == Triple::Apple)
----------------
I think the comment is referring to the vendor part of the triple, which is `apple`, so lowercase is better. Maybe could be in quotes though, something like "If vendor is 'apple'", but I'm not sure this is worth changing.


================
Comment at: llvm/lib/Support/Triple.cpp:1683
       return VersionTuple(14, 0, 0);
-    // ARM64e slice is supported starting from iOS 14.
+    // ARM64e slice is supported starting from iOS 14+.
     if (isArm64e())
----------------
The `+` seems redundant, since "starting from" should cover it.


================
Comment at: llvm/lib/Support/Triple.cpp:1767
-
-  llvm_unreachable("invalid arch name");
 }
----------------
jrtc27 wrote:
> Don't include unrelated changes
Why remove this?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107970