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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: clang/include/clang/Basic/DarwinSDKInfo.h:129
+  static Optional<DarwinSDKInfo>
+  parseDarwinSDKSettingsJSON(StringRef FilePath, const llvm::json::Object *Obj);
+
----------------
dexonsmith wrote:
> Should this take the VFS?
No, the Filepath is not actually used here. I removed it.


================
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);
----------------
aaron.ballman wrote:
> Why are you looking for a minor key here?
To avoid recursing infinitely in the next iteration, when minor is 0.


================
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?
----------------
aaron.ballman wrote:
> Does this scenario happen often (like, is warning the user about this situation a bad idea without a default-off warning flag)?
It shouldn't ever happen unless the SDKs are damaged. It would still probably be good to warn about it, so I added an extra warning.


================
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")
----------------
aaron.ballman wrote:
> 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.
I can do this in a followup NFC patch.


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

https://reviews.llvm.org/D105257