dexonsmith added a subscriber: aaron.ballman. dexonsmith added a comment. Thanks for splitting this out! It mostly looks good, just a few things inline.
================ Comment at: clang/lib/Basic/DarwinSDKInfo.cpp:27-28 + return KV->getSecond(); + // If no exact entry found, try just the major key version. + if (Key.getMinor()) + return map(VersionTuple(Key.getMajor()), MinimumValue, MaximumValue); ---------------- Your answer (https://reviews.llvm.org/D105257#inline-1007563) to @aaron.ballman's comment in the original patch was informative: this check is to avoid infinite recursion. That wasn't obvious to me either though. Might you add something to the comment for the benefit of other readers? ================ Comment at: clang/lib/Basic/DarwinSDKInfo.cpp:116 if (const auto *Obj = Result->getAsObject()) { + // FIXME: Switch to use parseDarwinSDKSettingsJSON in the next commit. auto VersionString = Obj->getString("Version"); ---------------- Nit: I'd suggest skipping the "in the next commit" part. Instead, I'd suggest documenting in the commit message why this wasn't done here: - This patch adds/tests new low-level functionality. - Future commit will adopt it in `parseDarwinSDKInfo` and actually change the driver/frontend/whatever. ================ Comment at: clang/unittests/Basic/DarwinSDKinfoTest.cpp:35-49 + EXPECT_EQ(*Mapping->map(VersionTuple(10, 15), VersionTuple(), None), + VersionTuple(13, 1)); + EXPECT_EQ(*Mapping->map(VersionTuple(11, 0), VersionTuple(), None), + VersionTuple(14, 0)); + EXPECT_EQ(*Mapping->map(VersionTuple(11, 2), VersionTuple(), None), + VersionTuple(14, 2)); + ---------------- Can you add a couple of comments saying what the EXPECTs are testing? (Maybe not each one individually, but at least groups by what they're testing (maybe exists, fallback to major, infinite loop, big minimum value, etc., but in full sentences)). Should there be a test that passes both minimum and maximum values? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105958/new/ https://reviews.llvm.org/D105958 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
