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