Page MenuHomePhabricator

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

Authored by compnerd on Sep 15 2020, 1:27 PM.

Details

Summary

This attribute allows declarations to be restricted to the framework
itself, enabling Swift to remove the declarations when importing
libraries. This is useful in the case that the functions can be
implemented in a more natural way for Swift.

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 15 2020, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2020, 1:27 PM
compnerd requested review of this revision.Sep 15 2020, 1:27 PM
aaron.ballman added inline comments.Sep 17 2020, 7:22 AM
clang/include/clang/Basic/Attr.td
2189

Similar concerns here with listing all of the subjects as with the other review? (I don't have strong opinions.)

clang/lib/Sema/SemaDecl.cpp
2613–2614 ↗(On Diff #292014)

Hmm, I'm a bit confused. The attribute is an inheritable attribute, but when we try to merge attributes between declarations, we drop it?

clang/test/SemaObjC/attr-swift-private.m
1 ↗(On Diff #292014)

No need for blocks support?

25 ↗(On Diff #292014)

Should also have tests for the subjects you expect not to work.

gribozavr2 added inline comments.Sep 18 2020, 2:32 PM
clang/include/clang/Basic/AttrDocs.td
3592

Please drop the "Objective-C" part.

3597

..., while hiding the non-idiomatic one.

compnerd marked 4 inline comments as done.Thu, Sep 24, 11:47 AM
compnerd added inline comments.
clang/include/clang/Basic/Attr.td
2189

Yes, I think that is a justified concern and it would be best to not have the subject list as per the swift_name case.

clang/test/SemaObjC/attr-swift-private.m
25 ↗(On Diff #292014)

This no longer makes sense with the subject list removed.

compnerd updated this revision to Diff 294124.Thu, Sep 24, 11:48 AM
compnerd marked an inline comment as done.
compnerd marked an inline comment as done.Thu, Sep 24, 12:53 PM
compnerd updated this revision to Diff 294140.Thu, Sep 24, 12:55 PM

add more tests to cover the confusing case of attribute merge handling

compnerd marked an inline comment as done.Thu, Sep 24, 2:28 PM
compnerd added inline comments.
clang/lib/Sema/SemaDecl.cpp
2613–2614 ↗(On Diff #292014)

Discussed offline - this is dead code. Removed and added a test case to demonstrate the beahviour.

compnerd updated this revision to Diff 294167.Thu, Sep 24, 2:29 PM
compnerd marked an inline comment as done.
  • address all feedback, add more test cases
This revision is now accepted and ready to land.Fri, Sep 25, 8:46 AM