ProtocolConformanceAnalysis #17177
Merged
ProtocolConformanceAnalysis #17177
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
slavapestov
reviewed
Jun 13, 2018
| virtual void invalidateFunctionTables() override {} | ||
|
|
||
| /// Get the nominal types that implement a protocol. | ||
| const NominalTypeList &getConformances(ProtocolDecl *P) { |
There was a problem hiding this comment.
Should this method itself be const?
slavapestov
reviewed
Jun 13, 2018
|
|
||
| /// Get the nominal types that implement a protocol. | ||
| const NominalTypeList &getConformances(ProtocolDecl *P) { | ||
| return ProtocolConformanceCache[P]; |
There was a problem hiding this comment.
Can you use find() instead?
|
This looks great, I only have two minor nitpicks. |
rajbarik
force-pushed
the
raj-protoconfanal
branch
from
2e8fbab to
aa368ca
Compare
|
@slavapestov have a look. |
slavapestov
reviewed
Jun 16, 2018
| if (auto *NTD = dyn_cast<NominalTypeDecl>(D)) { | ||
| auto Protocols = NTD->getAllProtocols(); | ||
| for (auto &Protocol : Protocols) { | ||
| ProtocolConformanceCache[Protocol].push_back(NTD); |
There was a problem hiding this comment.
Please move the 'access level > internal' check up here
|
|
||
| public: | ||
| NominalTypeWalker(ProtocolConformanceAnalysis::ProtocolConformanceMap | ||
| &ProtocolConformanceCache) |
There was a problem hiding this comment.
Nitpick: This should line up with opening (
| void ProtocolConformanceAnalysis::init() { | ||
|
|
||
| // We only do this in Whole-Module compilation mode. | ||
| if (!(M->isWholeModule())) |
| /// or lower in access levels at a whole-module compilation mode. | ||
| if (!ProtocolConformanceCache.count(ProtoDecl)) | ||
| continue; | ||
| if (ProtoDecl->getEffectiveAccess() > AccessLevel::Internal) |
There was a problem hiding this comment.
Remove this (see above)
| const NominalTypeList *getConformances(const ProtocolDecl *P) const { | ||
| auto ConformsListIt = ProtocolConformanceCache.find(P); | ||
| return ConformsListIt != ProtocolConformanceCache.end() | ||
| ? reinterpret_cast<const NominalTypeList *>(&*ConformsListIt) |
There was a problem hiding this comment.
I don't think the cast here is necessary
slavapestov
reviewed
Jun 16, 2018
| virtual void invalidateFunctionTables() override {} | ||
|
|
||
| /// Get the nominal types that implement a protocol. | ||
| const NominalTypeList *getConformances(const ProtocolDecl *P) const { |
There was a problem hiding this comment.
It might be better to return an ArrayRef<NominalTypeDecl *> here instead of a pointer to a SmallVector
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
rajbarik
force-pushed
the
raj-protoconfanal
branch
from
aa368ca to
9c1817c
Compare
|
These are excellent suggestions @slavapestov! Please have a look now. |
rajbarik
force-pushed
the
raj-protoconfanal
branch
from
9c1817c to
ab95ea2
Compare
slavapestov
reviewed
Jun 18, 2018
|
|
||
| bool walkToDeclPre(Decl *D) override { | ||
| if (auto *NTD = dyn_cast<NominalTypeDecl>(D)) { | ||
| if (NTD->getEffectiveAccess() <= AccessLevel::Internal) { |
There was a problem hiding this comment.
Isn't the right check here for an internal protocol, not an internal conforming type? Public protocols can always have conforming types from outside the module.
… types (classes, structs, and enums) that conform to it. It is performed in the whole-module-compilation mode.
rajbarik
force-pushed
the
raj-protoconfanal
branch
from
ab95ea2 to
c2aef11
Compare
|
The check is both ways and is dependent on where you want to add the check.
I have updated it to carry only the non-public protocol information in the
cache.
…On Mon, Jun 18, 2018 at 1:08 PM, Slava Pestov ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/SILOptimizer/Analysis/ProtocolConformanceAnalysis.cpp
<#17177 (comment)>:
> +
+using namespace swift;
+
+namespace {
+/// A helper class to collect all nominal type declarations.
+class NominalTypeWalker : public ASTWalker {
+ ProtocolConformanceAnalysis::ProtocolConformanceMap &ProtocolConformanceCache;
+
+public:
+ NominalTypeWalker(ProtocolConformanceAnalysis::ProtocolConformanceMap
+ &ProtocolConformanceCache)
+ : ProtocolConformanceCache(ProtocolConformanceCache) {}
+
+ bool walkToDeclPre(Decl *D) override {
+ if (auto *NTD = dyn_cast<NominalTypeDecl>(D)) {
+ if (NTD->getEffectiveAccess() <= AccessLevel::Internal) {
Isn't the right check here for an internal *protocol*, not an internal
conforming type? Public protocols can always have conforming types from
outside the module.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#17177 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFBwaZzQIjvB66-61YZqrKvbWRwvnpbDks5t-AjCgaJpZM4Umpc7>
.
|
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
|
@rajbarik Thanks for making the changes. The PR looks good now, I'm going to merge it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ProtocolConformanceAnalysis for a non-public protocol returns all the types (classes, structs, and enums) that conform to it. It is performed in the whole-module-compilation mode.
It is part one (only analysis phase) of the PR at #16739.