This is an archive of the discontinued LLVM Phabricator instance.

Sema: add support for `__attribute__((__swift_bridged_typedef__))`
ClosedPublic

Authored by compnerd on Sep 9 2020, 10:11 AM.

Details

Summary

Extend the semantic attributes that clang processes for Swift to include
swift_bridged_typedef. This attribute enables typedefs to be bridged
into Swift with a bridged name.

This is based on the work of the original changes in
https://github.com/llvm/llvm-project-staging/commit/8afaf3aad2af43cfedca7a24cd817848c4e95c0c

Diff Detail

Event Timeline

compnerd created this revision.Sep 9 2020, 10:11 AM
Herald added a project: Restricted Project. · View Herald Transcript
compnerd requested review of this revision.Sep 9 2020, 10:11 AM
aaron.ballman added inline comments.Sep 10 2020, 11:03 AM
clang/include/clang/Basic/Attr.td
2133

Should this be inherited on redeclarations?

2135

Does the default diagnostic text generate something bad that caused you to add typedefs here?

clang/include/clang/Basic/AttrDocs.td
3484

I think it would be helpful to show the String vs NSString behavior in a short code example of how to use the attribute.

clang/lib/Sema/SemaDeclAttr.cpp
7537

Should there be any type checking that the underlying type of the typedef is a sensible one to bridge?

clang/test/SemaObjC/attr-swift_bridged_typedef.m
9

Please also add some examples where the attribute has arguments. Also, should this work in Objective-C++ when using a using type alias? If so, an example showing that working would also be helpful.

compnerd added inline comments.Sep 11 2020, 10:52 AM
clang/include/clang/Basic/Attr.td
2133

I don't see why not. @rjmccall, @doug.gregor any reason to not permit this to be inherited by redeclarations?

2135

I don't remember, I'll double check.

clang/lib/Sema/SemaDeclAttr.cpp
7537

I can't really think of anything that you could check that would be valuable. What types of things were you thinking?

clang/test/SemaObjC/attr-swift_bridged_typedef.m
9

I don't know about the ObjC++11 case with the using. @doug.gregor or @rjmccall?

compnerd marked 2 inline comments as done.Sep 11 2020, 2:55 PM
compnerd updated this revision to Diff 291346.Sep 11 2020, 2:57 PM
compnerd edited the summary of this revision. (Show Details)

Address feedback

aaron.ballman added inline comments.Sep 14 2020, 5:54 AM
clang/include/clang/Basic/Attr.td
2135

I noticed on a few other reviews that there was a field there, I wonder if the code originated before we added smarter logic for arbitrary subject lists.

clang/lib/Sema/SemaDeclAttr.cpp
7537

I was mostly thinking about builtin types like int, but I don't insist.

compnerd added inline comments.Sep 14 2020, 8:12 AM
clang/include/clang/Basic/Attr.td
2135

Yes, this code is definitely old. If there are sites that you notice that could be changed to something new, Im happy to work though those changes.

compnerd marked an inline comment as done.Sep 14 2020, 8:28 AM
compnerd added inline comments.
clang/lib/Sema/SemaDeclAttr.cpp
7537

Typedefing int to something else and importing that with a different name is reasonable I believe. I'll add a test case.

aaron.ballman added inline comments.Sep 14 2020, 8:46 AM
clang/lib/Sema/SemaDeclAttr.cpp
7537

Okay, perfect, thank you for verifying!

compnerd updated this revision to Diff 291587.Sep 14 2020, 9:04 AM

Add additional test case

compnerd marked an inline comment as done.Sep 14 2020, 10:44 AM
compnerd updated this revision to Diff 291621.Sep 14 2020, 11:06 AM
  • Add additional test case that was requested
compnerd retitled this revision from Sema: add support for `__attribute__((__swift_typedef_bridged__))` to Sema: add support for `__attribute__((__swift_bridged_typedef__))`.Sep 14 2020, 12:57 PM
compnerd edited the summary of this revision. (Show Details)
compnerd marked 3 inline comments as done.Sep 14 2020, 2:31 PM
compnerd added inline comments.
clang/include/clang/Basic/Attr.td
2133

Checked, this should be inheritable, added a test case too.

compnerd updated this revision to Diff 291687.Sep 14 2020, 2:32 PM
  • make the attribute inheritable
  • add a test case for inheritance
  • add a test case for C++ type alias
aaron.ballman accepted this revision.Sep 15 2020, 5:10 AM

LGTM with some test location nits.

clang/test/SemaObjC/attr-swift_bridged_typedef.m
3

Can you split this bit off into a separate test in the AST directory? Then you don't have to contend with the errors in the test file and it's a more realistic test (AST tests with errors present always leave me wondering how the AST will change as we add error recovery functionality to the AST).

clang/test/SemaObjC/attr-swift_bridged_typedef.mm
1 ↗(On Diff #291687)

Can you move this test to the AST directory as well?

This revision is now accepted and ready to land.Sep 15 2020, 5:10 AM
compnerd marked 2 inline comments as done.Sep 15 2020, 9:31 AM