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