[SourceKit] Pass ResolvedCursorInfo as shared pointer instead of by value
#63612
Conversation
|
@swift-ci Please smoke test |
|
I haven't looked through it all, but is there some reason this can't be a |
| @@ -392,13 +395,13 @@ void swift::simple_display(llvm::raw_ostream &out, const CursorInfoOwner &owner) | |||
| } | |||
|
|
|||
| void swift::ide::simple_display(llvm::raw_ostream &out, | |||
| const ide::ResolvedCursorInfo &info) { | |||
| if (info.isInvalid()) | |||
| ide::ResolvedCursorInfoPtr info) { | |||
There was a problem hiding this comment.
Seems weird to pass the shared pointer here given it just does a bunch of accesses, which I imagine is the case for most of the uses. Just pass const* instead?
Ah. We cache the result, so we pretty much need to use a shared pointer here. Fair enough. |
| dyn_cast<ResolvedValueRefCursorInfo>(&CursorInfo); | ||
| dyn_cast_cursor_info<ResolvedValueRefCursorInfo>(CursorInfo); |
There was a problem hiding this comment.
dyn_cast can take an intrusive pointer and returns a *. I don't think we need dyn_cast_cursor_info.
| ResolvedCursorInfoPtr CursorInfo = | ||
| evaluateOrDefault(SrcFile.getASTContext().evaluator, | ||
| CursorInfoRequest{CursorInfoOwner(&SrcFile, Loc)}, | ||
| new ResolvedCursorInfo()); |
There was a problem hiding this comment.
I'd prefer this just be nullptr and remove the invalid state of ResolvedCursorInfo. Happy for that to be in a different PR though.
ahoppen
force-pushed
the
ahoppen/resolved-cursor-info-by-reference
branch
from
015dedb to
464d38b
Compare
… value This allows us to model the `ResolvedCursorInfo` types as a proper type hierarchy instead of having to store all values in the base `ResolvedCursorInfo` type. rdar://102853071
ahoppen
force-pushed
the
ahoppen/resolved-cursor-info-by-reference
branch
from
464d38b to
ee26b4b
Compare
|
@swift-ci Please smoke test |
| const ResolvedValueRefCursorInfo &Info, bool AddRefactorings, | ||
| ResolvedValueRefCursorInfoPtr Info, bool AddRefactorings, |
There was a problem hiding this comment.
This is the only use of ResolvedValueRefCursorInfoPtr right? It could theoretically stay as a const ResolvedValueRefCursorInfo &Info and then just stack allocate ResolvedValueRefCursorInfo in the two callers. But also I strongly dislike all the callbacks here, so maybe this will make it easier to get rid of that later 🤷
| if (Result) { | ||
| Consumer.handleResults(*CursorInfo); | ||
| Consumer.handleResults(CursorInfo); | ||
| } |
There was a problem hiding this comment.
CursorInfo could have been null here before 😅. I'm honestly not sure why it doesn't actually cause any issues though. It does on Linux with C++ stdlib assertions enabled. Anyway, looks like this fixes it (assuming handleResults handles nullptr).
|
@swift-ci Please smoke test macOS |
As discussed in #62292 (comment).
This allows us to model the
ResolvedCursorInfotypes as a proper type hierarchy instead of having to store all values in the baseResolvedCursorInfotype.rdar://102853071