[NFC] Convert the Templates Computing DependencyKeys to the Builder Pattern #36096
Conversation
…attern It can be quite difficult to tell at a glance just how any particular decl is going to be converted into a key. The space of available template specializations is also 2-dimensional which adds an additional level of difficulty when the time comes to extend or refactor any of them. Unroll all of the templates into a builder that coalesces the commonalities of the ways DependencyKeys are built to combat this.
|
@swift-ci smoke test |
|
@swift-ci smoke test |
There was a problem hiding this comment.
It does make the code more understandable. My one concern is that I think this cleanup may cost at least one, and maybe more dynamic dispatches into the switch statements for name and context. Another option could be to return both name and context in one pair. One could argue that another few dispatches on the key type are insignificant compared to the compile time, though. What do you think? (I guess I'm ok either way.)
| return enumerateUse<NodeKind::member>(context, name, enumerator); | ||
| } | ||
| } | ||
| // Assume that what is depended-upon is the interface |
There was a problem hiding this comment.
A reader could need more info explaining that assumption. Why couldn't an implementation be depended-upon?
There was a problem hiding this comment.
I see... you just moved an old comment and--my bad!--it was my obscure old comment!
There was a problem hiding this comment.
Still, this feels like a non-centralized invariant. I think the other part must be in the code that records uses.
Is there any way to centralize it?
Here's the sort of scenario that I'm worried about: Suppose there's a class with private methods. Adding a private method would change the implementation but not the interface. But some other thing that depended in the vtable might need to depend on the interface. (Just thinking out loud here.)
There was a problem hiding this comment.
The "distributed invariant" is also a concern, but really independent of this PR.
| DependencyKey::Builder DependencyKey::Builder::withName(const Decl *decl) && { | ||
| switch (kind) { | ||
| case NodeKind::topLevel: { | ||
| auto name = getTopLevelName(decl); | ||
| return Builder{kind, aspect, context, name}; | ||
| } | ||
| case NodeKind::dynamicLookup: { | ||
| auto name = cast<ValueDecl>(decl)->getBaseName().userFacingName(); | ||
| return Builder{kind, aspect, context, name}; | ||
| } | ||
| case NodeKind::nominal: | ||
| case NodeKind::potentialMember: | ||
| case NodeKind::member: | ||
| case NodeKind::externalDepend: | ||
| case NodeKind::sourceFileProvide: | ||
| return Builder{kind, aspect, context, ""}; | ||
| case NodeKind::kindCount: | ||
| llvm_unreachable("saw count!"); | ||
| } | ||
| } | ||
|
|
||
| template <> | ||
| std::string | ||
| DependencyKey::computeNameForProvidedEntity<NodeKind::topLevel, | ||
| PrecedenceGroupDecl const *>( | ||
| const PrecedenceGroupDecl *D) { | ||
| return D->getName().str().str(); | ||
| } | ||
| template <> | ||
| std::string DependencyKey::computeNameForProvidedEntity< | ||
| NodeKind::topLevel, FuncDecl const *>(const FuncDecl *D) { | ||
| return getBaseName(D); | ||
| } | ||
| template <> | ||
| std::string DependencyKey::computeNameForProvidedEntity< | ||
| NodeKind::topLevel, OperatorDecl const *>(const OperatorDecl *D) { | ||
| return D->getName().str().str(); | ||
| } | ||
| template <> | ||
| std::string DependencyKey::computeNameForProvidedEntity< | ||
| NodeKind::topLevel, NominalTypeDecl const *>(const NominalTypeDecl *D) { | ||
| return D->getName().str().str(); | ||
| } | ||
| template <> | ||
| std::string DependencyKey::computeNameForProvidedEntity< | ||
| NodeKind::topLevel, ValueDecl const *>(const ValueDecl *D) { | ||
| return getBaseName(D); | ||
| } | ||
| template <> | ||
| std::string DependencyKey::computeNameForProvidedEntity< | ||
| NodeKind::dynamicLookup, ValueDecl const *>(const ValueDecl *D) { | ||
| return getBaseName(D); | ||
| } | ||
| template <> | ||
| std::string DependencyKey::computeNameForProvidedEntity< | ||
| NodeKind::nominal, NominalTypeDecl const *>(const NominalTypeDecl *D) { | ||
| return ""; | ||
| } | ||
| template <> | ||
| std::string | ||
| DependencyKey::computeNameForProvidedEntity<NodeKind::potentialMember, | ||
| NominalTypeDecl const *>( | ||
| const NominalTypeDecl *D) { | ||
| return ""; | ||
| DependencyKey::Builder DependencyKey::Builder::withName( | ||
| std::pair<const NominalTypeDecl *, const ValueDecl *> holderAndMember) && { | ||
| return std::move(*this).withName( | ||
| holderAndMember.second->getBaseName().userFacingName()); |
There was a problem hiding this comment.
I sympathize with--if I'm reading this right--replacing a passel-o-templates with a switch. But--sigh--does this replacement cost an extra dynamic dispatch? And if so, should we care?
There was a problem hiding this comment.
I believe we have much more rich optimization targets to go for than this. There's the cost of all the string copies in the dependency graph, for example, that we really ought to shoot for at some point...
| DependencyKey::Builder DependencyKey::Builder::withContext(const Decl *D) && { | ||
| switch (kind) { | ||
| case NodeKind::nominal: | ||
| case NodeKind::potentialMember: | ||
| case NodeKind::member: | ||
| /// nominal and potential member dependencies are created from a Decl and | ||
| /// use the context field. | ||
| return Builder{kind, aspect, cast<NominalTypeDecl>(D), name}; | ||
| case NodeKind::topLevel: | ||
| case NodeKind::dynamicLookup: | ||
| case NodeKind::externalDepend: | ||
| case NodeKind::sourceFileProvide: | ||
| // Context field is not used for most kinds | ||
| return Builder{kind, aspect, nullptr, name}; | ||
| case NodeKind::kindCount: | ||
| llvm_unreachable("saw count!"); | ||
| } |
There was a problem hiding this comment.
Same comment as withName below.
|
@swift-ci smoke test Windows |
|
⛵ |
It can be quite difficult to tell at a glance just how any particular decl is going to be converted into a key. The space of available template specializations is also 2-dimensional which adds an additional level of difficulty when the time comes to extend or refactor any of them. Unroll all of the templates into a builder that coalesces the commonalities of the ways DependencyKeys are built to combat this.