Skip to content

[SourceKit] Share common object printer code #3163

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

Conversation

briancroom
Copy link
Contributor

@briancroom briancroom commented Jun 23, 2016

What's in this pull request?

I thought it would make sense to open a new PR for a subset of the changes being discussed in #3026.

This is my attempt at implementing @akyrtzi's suggestion here. I didn't see how it would be possible to do it exactly as proposed, since the SKDObjectPrinterBase class needs access to the visit method to dispatch based on the type of arbitrary elements in a dictionary or array. I used a slightly different structure here which seemed to make sense, however the previous design of the classes SKDObjectVisitor and SKDObjectPrinter classes seemed confusing to me to begin with, so it's quite possible that some original design goals haven't been preserved with my changes.

I also noticed that the code duplication being discussed in relation to the InProc additions in #3026 already existed with the XPC SKDObjectPrinter and the VariantPrinter classes which I am addressing here.

Related bug number: (SR-1639)


Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

This is an attempt to clean up code duplication around printing SourceKit
request and response objects.

@briancroom
Copy link
Contributor Author

@swift-ci please test

@@ -177,6 +179,83 @@ static inline sourcekitd_variant_t makeUIDVariant(sourcekitd_uid_t value) {
return {{ 0, (uintptr_t)value, SOURCEKITD_VARIANT_TYPE_UID }};
}

template <typename VisitorImplClass, typename VisitedType>
class RequestResponsePrinterBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend to split RequestResponsePrinterBase to its own header. Internal.h is getting large.

This is an attempt to clean up code duplication around printing SourceKit
request and response objects.
@briancroom briancroom force-pushed the sourcekit-shared-object-printer branch from c4497dc to 5a30003 Compare June 24, 2016 02:01
@briancroom
Copy link
Contributor Author

I finally got it now! Sorry you for making you spell it out in such detail 😬 I don't think I've encountered quite this pattern before.

@briancroom
Copy link
Contributor Author

@swift-ci Please test and merge

@swift-ci swift-ci merged commit 3ea89df into swiftlang:master Jun 24, 2016
@briancroom briancroom deleted the sourcekit-shared-object-printer branch June 24, 2016 03:57
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