Skip to content
This repository was archived by the owner on Mar 28, 2020. It is now read-only.

Conversation

@jrose-apple
Copy link
Contributor

...by wrapping them in another (new) attribute, SwiftVersionedAttr. This can track both attributes that would be added and those that would be removed.

This doesn't support other kinds of changes that can be made via API notes, such as nullability or wholesale type changes, but it's a place to start, and possibly sufficient for our goals.

Part of rdar://problem/28618121.

@jrose-apple
Copy link
Contributor Author

Also cherry-pick a fix that accidentally went into 'stable' instead of upstream-with-swift. @hughbe, can you cherry-pick that to swift-3.1-branch too?

@jrose-apple
Copy link
Contributor Author

Factored that out into #54.

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is simpler than I'd expected, which is fantastic. LGTM!

@hughbe
Copy link
Collaborator

hughbe commented Dec 14, 2016

@jrose-apple I'm fairly sure these changes went into swift-3.1-branch... see: #45

Also, the apple/swift-3.1-branch seems up to date with my changes

@jrose-apple
Copy link
Contributor Author

Ah, strange. I guess I checked the wrong thing with git. We did miss upstream-with-swift, though—oops!

@jrose-apple jrose-apple force-pushed the SwiftVersionedAttr branch 3 times, most recently from 2347d96 to 037afa9 Compare December 23, 2016 01:15
...by wrapping them in another (new) attribute, SwiftVersionedAttr,
or by noting their deletion in a SwiftVersionedRemovalAttr.

This doesn't support other kinds of changes that can be made via API
notes, such as nullability or wholesale type changes, but it's a place
to start, and possibly sufficient for our goals.

Part of rdar://problem/28618121.
*existing);

D->getAttrs().erase(existing.getCurrent());
D->addAttr(versioned);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DougGregor: This part is new since last time. Do you see any issues with this version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess an empty VersionTuple is code for "latest version". That seems fine to me, and this looks like the right way to maintain the attribute that was present in the source code without the rest of Clang (or other tools that only care about the API-note-adjusted view) seeing it.

@jrose-apple
Copy link
Contributor Author

@fredriss, what do you think about this? This particular PR is going to 'upstream-with-swift', but I'd like to land the corresponding Swift changes soon after. Is it worth cherry-picking to Clang's 'swift-3.1-branch', or should I limit it to the 'stable' branch and let it get picked up in the next Clang rebranch (presumably for Swift 4)?

*existing);

D->getAttrs().erase(existing.getCurrent());
D->addAttr(versioned);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess an empty VersionTuple is code for "latest version". That seems fine to me, and this looks like the right way to maintain the attribute that was present in the source code without the rest of Clang (or other tools that only care about the API-note-adjusted view) seeing it.

@jrose-apple jrose-apple merged commit f8c16c2 into apple:upstream-with-swift Jan 11, 2017
@jrose-apple jrose-apple deleted the SwiftVersionedAttr branch January 11, 2017 23:02
jrose-apple added a commit to jrose-apple/swift-clang that referenced this pull request Jan 11, 2017
...by wrapping them in another (new) attribute, SwiftVersionedAttr,
or by noting their deletion in a SwiftVersionedRemovalAttr.

This doesn't support other kinds of changes that can be made via API
notes, such as nullability or wholesale type changes, but it's a place
to start, and possibly sufficient for our goals.

Part of rdar://problem/28618121.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants