[SourceKit] In-Proc implementation of sourcekitd API functions #3026
Conversation
|
@swift-ci please test |
briancroom
force-pushed
the
sourcekit-inproc-API
branch
from
81eb4b1 to
93f525d
Compare
|
The "Move some functions into sourcekitdAPI-Common" commit LGTM on its own, could you have it as a separate pull request so we can test & merge it as is ? |
|
Sounds good: #3096. Once that goes in, I'll get this rebased. I've not written much C++ lately so I'm sure there's much to be improved here! I'm looking forward to specific feedback when you get a chance to take a look. |
| --RetainCount; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
llvm has helpers to define and use ref-counted objects, see "llvm/ADT/IntrusiveRefCntPtr.h" and "swift/Basic/ThreadSafeRefCounted.h" that introduces ThreadSafeRefCountedBaseVPTR.
Essentially you can do this:
class SKDObject : public ThreadSafeRefCountedBaseVPTR {
This will give you an object (with a virtual destructor) that can be passed around as ref-counted pointer using llvm::IntrusiveRefCntPtr<T>.
briancroom
force-pushed
the
sourcekit-inproc-API
branch
from
93f525d to
c645205
Compare
|
Thanks for the schooling in LLVM patterns @akyrtzi. I've pushed a first attempt at reworking the code using LLVM-style refcounting and RTTI. Notes:
|
| using llvm::ArrayRef; | ||
| using llvm::StringRef; | ||
| using llvm::raw_ostream; | ||
|
|
There was a problem hiding this comment.
These using declarations shouldn't be necessary since "SourceKit/Core/LLVM.h" is included.
|
LGTM, with exception of the nitpicks and suggestion to see if |
|
Thanks for the thorough review and great feedback! This is very helpful. |
briancroom
force-pushed
the
sourcekit-inproc-API
branch
from
c645205 to
f533cc5
Compare
|
Updated to reflect the comments, except for the duplication around |
briancroom
force-pushed
the
sourcekit-inproc-API
branch
from
f533cc5 to
080fd2d
Compare
|
@swift-ci please test |
|
Ok, updated again to take advantage of the @akyrtzi do you have any more feedback before this goes in? |
| virtual size_t getCount() const { return 0; } | ||
|
|
||
| virtual sourcekitd_uid_t getUID() const { return nullptr; } | ||
| virtual Optional<int64_t> getInt64() const { return Optional<int64_t>(); } |
There was a problem hiding this comment.
The idiom to use when wanting to pass an Optional without value is to use llvm::None:
virtual Optional<int64_t> getInt64() const { return None; }
There are other instances like this in the rest of the file.
There was a problem hiding this comment.
Ahhh nice. I was looking for something like that but couldn't track it down before.
|
Other than a couple of extra nitpicks, LGTM! |
Darwin can use the XPC implementation of this even for the in-proc library build, this alternative implementation is need for Linux where XPC services are not available. This implementation is derived from the existing XPC one but replaces XPC services' object model with a basic class hierarchy providing reference-counted objects for the value types and collections that compose the sourcekitd API's request and response types.
briancroom
force-pushed
the
sourcekit-inproc-API
branch
from
080fd2d to
2c4eb84
Compare
|
Excellent! I'm going to go ahead and merge right away now after having made those last updates since CI doesn't actually build this file yet anyway. Thanks again for your support on this @akyrtzi |
|
Thanks for working on this! |
What's in this pull request?
The current sourcekitd API implementation is only available on Darwin because it relies on XPC services. Darwin uses this for both the standard and in-process build variations, but this alternative implementation is needed for Linux where XPC services are not available.
This implementation is derived from the existing XPC one but replaces XPC services' object model with a basic class hierarchy providing reference-counted objects for the value types and collections that compose the sourcekitd API's request and response types. It uses polymorphism to implement the RTTI required by the heterogenous collections.
Note that additional effort is required before SourceKit can build on Linux, but this is a significant piece of the puzzle. I'll trigger CI to smoke-test the changes, but the new code isn't touched by the CI build yet.
This supersedes #2769.
cc @modocache @gribozavr @akyrtzi
Resolved bug number: (SR-1639)
Before merging this pull request to apple/swift repository:
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
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.