This is an archive of the discontinued LLVM Phabricator instance.

[clang][darwin] add support for remapping macOS availability to Mac Catalyst availability
ClosedPublic

Authored by arphaman on Jun 30 2021, 7:56 PM.

Details

Summary

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.

Diff Detail

Event Timeline

arphaman created this revision.Jun 30 2021, 7:56 PM
arphaman requested review of this revision.Jun 30 2021, 7:56 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 30 2021, 7:56 PM
aaron.ballman added inline comments.Jul 1 2021, 4:33 AM
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?

dexonsmith added inline comments.Jul 1 2021, 10:34 AM
clang/lib/Sema/Sema.cpp
70

If you're not looking at the error payload, maybe you can call llvm::consumeError().

arphaman updated this revision to Diff 358511.Jul 13 2021, 10:04 PM
arphaman marked 11 inline comments as done.

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.

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).

I split the patch to also https://reviews.llvm.org/D105958 like you suggested.

dexonsmith accepted this revision.Jul 14 2021, 12:51 PM

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.

This revision is now accepted and ready to land.Jul 14 2021, 12:51 PM
aaron.ballman accepted this revision.Jul 15 2021, 4:03 AM

LGTM!

clang/lib/Sema/Sema.cpp
63

I don't have strong feelings either, but I think an early return would be reasonable.

arphaman marked 2 inline comments as done.Jul 21 2021, 11:32 AM
This revision was landed with ongoing or failed builds.Jul 21 2021, 11:32 AM
This revision was automatically updated to reflect the committed changes.