[PATCH] D105257: [clang][darwin] add support for remapping macOS availability to Mac Catalyst availability

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

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

LGTM, assuming @aaron.ballman is happy! (a couple of optional nits inline)



================
Comment at: clang/lib/Basic/DarwinSDKInfo.cpp:28
+  // If no exact entry found, try just the major key version.
+  if (Key.getMinor())
+    return map(VersionTuple(Key.getMajor()), MinimumValue, MaximumValue);
----------------
arphaman wrote:
> aaron.ballman wrote:
> > Why are you looking for a minor key here?
> To avoid recursing infinitely in the next iteration, when minor is 0.
In the split-out prep patch, I suggested adding a comment to that effect!


================
Comment at: clang/lib/Sema/Sema.cpp:63
+                                              StringRef Platform) {
+  if (!CachedDarwinSDKInfo) {
+    auto SDKInfo = parseDarwinSDKInfo(
----------------
Nit: I think I'd find this function easier to read if it used an early return here, duplicating the `CachedDarwinSDKInfo->get()` call. But I'm fine if you prefer it this way.


================
Comment at: clang/lib/Sema/Sema.cpp:67-70
+    if (SDKInfo && *SDKInfo)
+      CachedDarwinSDKInfo =
+          std::make_unique<DarwinSDKInfo>(std::move(**SDKInfo));
+    else {
----------------
Nit: I might refactor this to use an early return (could split out a helper function to wrap parseDarwinSDKInfo, or a lambda to wrap setting/returning CachedDarwinSDKInfo)...

... but this is fine too -- if you leave it as-is, please add braces to the `if` side to match the braces on the `else`.


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

https://reviews.llvm.org/D105257