[Target] Centralize creation of scratch SwiftASTContexts#1772
[Target] Centralize creation of scratch SwiftASTContexts#1772bulbazord wants to merge 1 commit intoapple:stablefrom
Conversation
|
@swift-ci please test |
|
friendly ping @JDevlieghere @dcci @adrian-prantl |
|
This makes sense to me. Not sure if @adrian-prantl, @JDevlieghere, or @dcci have any objections. |
JDevlieghere
left a comment
JDevlieghere
left a comment
There was a problem hiding this comment.
I'm not an expert on the functionality, but I'd prefer for us to improve error handling while we're around.
source/Target/Target.cpp
Outdated
|
|
||
| SwiftASTContextReader Target::GetScratchSwiftASTContext(ValueObject &valobj, | ||
| bool create_on_demand) { | ||
| Status error; |
There was a problem hiding this comment.
I don't like this error getting swallowed.
| Status error; | ||
| ExecutionContext ctx = valobj.GetExecutionContextRef().Lock(false); | ||
| auto *exe_scope = ctx.GetBestExecutionContextScope(); | ||
| return GetScratchSwiftASTContext(error, *exe_scope); |
There was a problem hiding this comment.
Can we make this return an llvm::Expected?
There was a problem hiding this comment.
Yeah I can do that :)
There was a problem hiding this comment.
Okay so I just looked into it. Looks like We always return a SwiftASTContextReader no matter what, so returning an llvm::Expected doesn't make much sense here. What I did find is that GetScratchSwiftASTContext will sometimes fail to build a scratch SwiftASTContext and instead return the project-wide scratch SwiftASTContext. Even if we fail to build a SwiftASTContext, we still return a SwiftASTContextReader.
There was a problem hiding this comment.
I have no strong preferences, so I'm happy with this once Jonas feels his comments have been addressed.
Maybe longer term we can enforce that the SwiftASTContextReader returned is always valid [and if it's not, assert], given we expect it in such state..
There was a problem hiding this comment.
Do we ever need to differentiate between the two? This code is saying that the error doesn't matter. Is that true for this function, or is that true always? In the latter case, maybe we should remove the Status as the first argument?
There was a problem hiding this comment.
From the looks of it, Target::GetScratchSwiftASTContext will return a Status and the callsites generally use them. It looks like error handling is just not taken into account with the ValueObject version. So even though it's true just for this function, I think that we can probably do a better job and improve error handling here by making the ValueObject version take a Status and then have the clients actually use that Status for error handling.
|
@JDevlieghere Mind taking another look when you get the chance? |
I don't think it makes sense for ValueObject to know about SwiftASTContexts, so we should centralize the creation logic into Target.
bulbazord
force-pushed
the
target_experiment
branch
from
c091272 to
91e5f2f
Compare
|
@swift-ci please test |
4 participants
I don't think it makes sense for ValueObject to know about SwiftASTContexts, so
we should centralize the creation logic into Target.
I know there are still references to SwiftASTContext in parts of ValueObject and its subclasses. I plan to try to factor those out sometime later.