Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upstream ns_error_domain attribute #1565

Merged
merged 2 commits into from
Aug 13, 2020
Merged

Upstream ns_error_domain attribute #1565

merged 2 commits into from
Aug 13, 2020

Conversation

MForster
Copy link

@MForster MForster commented Jul 30, 2020

There are two commits in this pull request:

The first commit proactively applies the changes that have been made to ns_error_domain during upstreaming to avoid merge conflicts when the change trickles down. This has been reviewed upstream at D84005, but not submitted, yet. If more comments come up for this part, I would apply them to the upstream code review.

The second commit adds additional changes for APInotes that have not been reviewed, yet.

@MForster
Copy link
Author

@gribozavr, @DougGregor, I would appreciate your review on this change.

@gribozavr
Copy link

LGTM!

@DougGregor could you review & merge?

@MForster
Copy link
Author

Thanks for the review. Please don't merge, yet. I think I may have found an issue. I'll mark this as draft while I investigate (tomorrow).

@MForster MForster marked this pull request as draft July 30, 2020 18:09
@MForster MForster marked this pull request as ready for review July 31, 2020 10:03
@MForster
Copy link
Author

This is ready for review (and merging) now.

@MForster
Copy link
Author

I've added an additional test from the upstream code review and rebased against apple/master head.

@DougGregor, can you review and merge this?

@MForster MForster requested a review from DougGregor August 10, 2020 15:13
@MForster
Copy link
Author

Hi @DougGregor, we do have commit rights for this branch now, so we can also move ahead on our own. We are still interested in your feedback, though, if you have time.

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.

I only had the chance to skim this, but it LGTM. Thank you!

This change proactively applies the changes that have been made to
ns_error_domain during upstreaming to avoid merge conflicts when the
change trickles down.

Original changelist description:

ns_error_domain can be used by, e.g. NS_ERROR_ENUM, in order to
identify a global declaration representing the domain constant.

Introduces the attribute, Sema handling, diagnostics, and test case.

This is cherry-picked from llvm/llvm-project-staging@a14779f
and adapted to updated Clang APIs.
This had been changed during upstreaming of the attribute and needs
some updates.
@MForster MForster merged commit f7bd8bf into swiftlang:apple/master Aug 13, 2020
@MForster MForster deleted the am/ns-errordomain branch August 13, 2020 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants