Skip to content

Sema: implement isFinal using a request evaluator #23932

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

Merged
merged 7 commits into from
Apr 19, 2019

Conversation

xymus
Copy link
Contributor

@xymus xymus commented Apr 10, 2019

Add the request evaluator IsFinalRequest to lazily determine if a ValueDecl is final.

@xymus xymus requested a review from slavapestov April 10, 2019 18:02
@xymus
Copy link
Contributor Author

xymus commented Apr 10, 2019

@swift-ci Please test source compatibility

Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

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

This looks wonderful. I only have a couple of minor questions and suggestions.

@slavapestov slavapestov self-assigned this Apr 10, 2019
@xymus
Copy link
Contributor Author

xymus commented Apr 10, 2019

I reworked IsFinalRequest::evaluate to handle separately the methods and accessors, it is cleaner and it fixed a few tests.

@slavapestov I'm looking into remaining issues with some getters that are incorrectly marked as 'final', or not marked when they should be. Let me know if you see something else that seems off with how the accessors are handled.

@@ -367,10 +367,6 @@ createCoroutineAccessorPrototype(AbstractStorageDecl *storage,
if (isStatic)
accessor->setStatic();

// The accessor is final if the storage is.
if (storage->isFinal())
makeFinal(ctx, accessor);
Copy link
Contributor

Choose a reason for hiding this comment

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

makeFinal() should be dead code now, you can remove it from its .cpp and .h files

Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. One final round of suggestions!

@xymus
Copy link
Contributor Author

xymus commented Apr 16, 2019

@swift-ci Please test source compatibility

@xymus
Copy link
Contributor Author

xymus commented Apr 16, 2019

@swift-ci Please smoke test

@xymus
Copy link
Contributor Author

xymus commented Apr 17, 2019

@swift-ci Please smoke test
@swift-ci Please test source compatibility

@xymus
Copy link
Contributor Author

xymus commented Apr 17, 2019

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor

@swift-ci Please smoke test Linux

1 similar comment
@slavapestov
Copy link
Contributor

@swift-ci Please smoke test Linux

@xymus xymus changed the title [WIP] Sema: implement isFinal using a request evaluator Sema: implement isFinal using a request evaluator Apr 19, 2019
@xymus xymus merged commit 007fbb6 into swiftlang:master Apr 19, 2019
@xymus xymus deleted the IsFinalRequest branch May 13, 2019 20:58
slavapestov added a commit to slavapestov/swift that referenced this pull request Aug 6, 2019
…ed members of a final class

I believe this was fixed with swiftlang#23932.

5.1 had an inconsistency here because the synthesized declaration was not
marked as final; we would sometimes check if a method itself was final,
other times we would check if both the method and the class were final.

Lucky it worked out so that the emitted vtable entry did not appear to
ever have been used.

Make sure this behavior doesn't change going forward with a test.
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