This patch adds supports for clang to remap macOS availability attributes that have introduced, deprecated or obsoleted versions to appropriate Mac Catalyst availability attributes. This mapping is done using the version mapping provided in the macOS SDK, in the SDKSettings.json file. The mappings in the SDKSettings json file will also be used in the clang driver for the driver Mac Catalyst patch, and they could also be used in the future for other platforms as well.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/include/clang/Basic/DarwinSDKInfo.h | ||
---|---|---|
40 ↗ | (On Diff #355754) | Should this be casting to StorageType? |
84 ↗ | (On Diff #355754) | |
clang/lib/Basic/DarwinSDKInfo.cpp | ||
19 | ||
28 | Why are you looking for a minor key here? | |
clang/lib/Sema/Sema.cpp | ||
72–75 | Does this scenario happen often (like, is warning the user about this situation a bad idea without a default-off warning flag)? | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
2570–2572 | 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. | |
2580–2581 | ||
2600–2602 | Can reflow the comments | |
llvm/include/llvm/Support/VersionTuple.h | ||
186–199 ↗ | (On Diff #355754) | Not following the usual naming conventions. |
Could the DarwinSDKInfo changes be tested directly with C++ unit tests? Since this stuff is in Basic it'd be nice to test it separately from the driver. If so, maybe they could also mostly land in a separate prep commit (except the update to clang::parseDarwinSDKInfo which is what changes the Driver behaviour).
clang/include/clang/Basic/DarwinSDKInfo.h | ||
---|---|---|
129 ↗ | (On Diff #355754) | Should this take the VFS? |
clang/lib/Sema/Sema.cpp | ||
---|---|---|
70 | If you're not looking at the error payload, maybe you can call llvm::consumeError(). |
Split patch and address review comments
clang/include/clang/Basic/DarwinSDKInfo.h | ||
---|---|---|
129 ↗ | (On Diff #355754) | No, the Filepath is not actually used here. I removed it. |
clang/lib/Basic/DarwinSDKInfo.cpp | ||
28 | To avoid recursing infinitely in the next iteration, when minor is 0. | |
clang/lib/Sema/Sema.cpp | ||
72–75 | 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. | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
2570–2572 | I can do this in a followup NFC patch. |
LGTM, assuming @aaron.ballman is happy! (a couple of optional nits inline)
clang/lib/Basic/DarwinSDKInfo.cpp | ||
---|---|---|
28 | In the split-out prep patch, I suggested adding a comment to that effect! | |
clang/lib/Sema/Sema.cpp | ||
63 | 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. | |
67–70 | 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. |
LGTM!
clang/lib/Sema/Sema.cpp | ||
---|---|---|
63 | I don't have strong feelings either, but I think an early return would be reasonable. |