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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DarwinSDKInfo.h:40
+                        llvm::Triple::EnvironmentType ToEnv)
+        : Value(((uint64_t(FromOS) * uint64_t(llvm::Triple::LastOSType) +
+                  uint64_t(FromEnv))
----------------
Should this be casting to `StorageType`?


================
Comment at: clang/include/clang/Basic/DarwinSDKInfo.h:84
+    map(const VersionTuple &Key, const VersionTuple &MinimumValue,
+        const Optional<VersionTuple> &MaximumValue) const;
+
----------------



================
Comment at: clang/lib/Basic/DarwinSDKInfo.cpp:19
+    const VersionTuple &Key, const VersionTuple &MinimumValue,
+    const Optional<VersionTuple> &MaximumValue) const {
+  if (Key < MinimumKeyVersion)
----------------



================
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);
----------------
Why are you looking for a minor key here?


================
Comment at: clang/lib/Sema/Sema.cpp:72-75
+                                // Ignore the error and pretend the SDK info
+                                // is missing.
+                                // FIXME: should we care about missing SDK info
+                                // for some darwin targets?
----------------
Does this scenario happen often (like, is warning the user about this situation a bad idea without a default-off warning flag)?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2565-2567
     if (II->getName() == "ios")
       NewII = &S.Context.Idents.get("maccatalyst");
     else if (II->getName() == "ios_app_extension")
----------------
This matches the surrounding code, but at some point, we may want to switch these to `isStr()` calls instead -- slightly more declarative as to what's going on.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2575-2576
+            (V.getMajor() == 13 && V.getMinor() && *V.getMinor() < 1))
+          return VersionTuple(13,
+                              1); // The minimum Mac Catalyst version is 13.1.
+        return V;
----------------



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2596-2598
+        // This inferred availability has
+        // lower priority than the other availability attributes that are
+        // inferred from 'ios'.
----------------
Can reflow the comments


================
Comment at: llvm/include/llvm/Support/VersionTuple.h:186-199
+    unsigned result = value.getMajor();
+    if (auto minor = value.getMinor())
+      result = detail::combineHashValue(result, *minor);
+    if (auto subminor = value.getSubminor())
+      result = detail::combineHashValue(result, *subminor);
+    if (auto build = value.getBuild())
+      result = detail::combineHashValue(result, *build);
----------------
Not following the usual naming conventions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105257