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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
