Skip to content

Conversation

@nkcsgexi
Copy link
Contributor

@nkcsgexi nkcsgexi commented Jul 25, 2018

rdar://42480588

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor Author

@nathawes could you please review this patch for swift-4.2-branch?

@nkcsgexi nkcsgexi requested a review from nathawes July 25, 2018 21:30
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this based purely on the function name at the moment?

If it is, it seems like this migration would cause errors if

  1. I have my own class with a similarly named, but unrelated method, e.g. 'changeColor(_ color: Color)', that I override in subclass (the subclass' override should be left alone)
  2. I have a subclass of NSObject and a subclass of that subclass that both override one of these methods (the sub-sub-class' method should still override).

Some of the names seem specific enough that the above wouldn't happen too often in practice, but I could imagine it happening for a few of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I think it is safer to use USR for identifying these overriding function. Will change!

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this check if the thing it's referencing is one of the targeted methods? E.g. I might be calling some unrelated method on NSObject that I also happen to override and I want to call the super class' version rather than my own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I'll add this check for these function invocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem like the right place for this one. E.g. I might also use super.changeColor() in methods other than my override of changeColor() if I don't want to use my override.

@nkcsgexi nkcsgexi force-pushed the 42480588 branch 4 times, most recently from 640cbb4 to 5ffb7a3 Compare July 26, 2018 19:31
@nkcsgexi
Copy link
Contributor Author

@nathawes I've addressed your comments above. Mind taking another look?

Copy link
Contributor

@nathawes nathawes left a comment

Choose a reason for hiding this comment

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

LGTM Xi! Thanks for updating it.

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi nkcsgexi merged commit 00c7cb6 into swiftlang:master Jul 27, 2018
akyrtzi added a commit that referenced this pull request Jul 29, 2018
This is #18226 but with a fix for the test to be macos specific.
rdar://42480588
nkcsgexi pushed a commit that referenced this pull request Jul 30, 2018
This is #18226 but with a fix for the test to be macos specific.
rdar://42480588
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.

2 participants