[CodeComplete] Make SourceKit::CodeCompletion::Completion store a reference to the underlying swift result instead of extending that type #40724
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
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test Linux |
rintaro
reviewed
Jan 10, 2022
| assert(SwiftContext.swiftASTContext); | ||
| Callback(ResultType::success({Results, SwiftContext})); | ||
| Callback(ResultType::success({context.getResultSink(), SwiftContext})); |
There was a problem hiding this comment.
This copies CodeCompletionResultSink object which has std::vector<CodeCompletionResult *> Results etc. Copying it sounds heavy. Do we really want that? If so could you explain why?
There was a problem hiding this comment.
Good catch CodeCompleteResult should store CodeCompletionResultSink by reference. Updated it.
Previously the code completion methods just returned an `ArrayRef` that pointed into the result sink that contained the results but no effort was made to actually keep that that result sink alive, e.g. when transforming results in `transformAndForwardResults`. Instead, return the `CodeCompletionResultSink` from the code compleiton methods now and adopt that sink from the inner results created in `transformAndForwardResults`.
…erence to the underlying swift result instead of extending that type Previously, when creating a `SourceKit::CodeCompletion::Completion`, we needed to copy all fields from the underlying `SwiftResult` (aka `swift::ide::CodeCompletionResult`). The arena in which the `SwiftResult` was allocated still needed to be kept alive for the references stored in the `SwiftResult`. To avoid this unnecessary copy, make `SourceKit::CodeCompletion::Completion` store a reference to the underlying `SwiftResult`.
|
@swift-ci Please smoke test |
ahoppen
force-pushed
the
pr/sourcekit-completion-reference-to-swift-result
branch
from
cb3e662 to
520c41a
Compare
|
@swift-ci Please smoke test Linux |
rintaro
approved these changes
Jan 13, 2022
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.
Previously, when creating a
SourceKit::CodeCompletion::Completion, we needed to copy all fields from the underlyingSwiftResult(akaswift::ide::CodeCompletionResult). The arena in which theSwiftResultwas allocated still needed to be kept alive for the references stored in theSwiftResult.To avoid this unnecessary copy, make
SourceKit::CodeCompletion::Completionstore a reference to the underlyingSwiftResult.While making this change, I also noticed that the
innerSinkcreated bytransformAndForwardResultsdid not adopt the sink of the outer results, but was still referencing e.g. strings stored in it. The first commit in this PR makes sure that we retain outer sink from the inner sink.