[rebranch] Fix various LLDB failures #3635
Conversation
|
@swift-ci please test |
|
@JDevlieghere I'll need to cherry-pick bc53247 to |
|
Added Jim for a second opinion, but looks good to me. |
|
@swift-ci please test |
|
|
||
| // BEGIN APPLE | ||
| llvm::Expected<TypeSystem &> | ||
| TypeSystemMap::GetTypeSystemForLanguage(lldb::LanguageType language, |
There was a problem hiding this comment.
I don't think the compiler options ever get used in the swift code anymore, so we don't really need this version either. It won't hurt anything to leave it in for now, but we should remove it at some point.
lldb/source/Symbol/TypeSystem.cpp
Outdated
| @@ -337,4 +346,4 @@ void TypeSystemMap::RemoveTypeSystemsForLanguage(lldb::LanguageType language) { | |||
| m_map.erase(pos); | |||
| } | |||
| } | |||
|
|
|||
| // END APPLE | |||
There was a problem hiding this comment.
We have been using // BEGIN SWIFT & // END SWIFT, not APPLE. Was there a reason this was switched to APPLE, or just an oversight. It would be better to be consistent with these markers.
There was a problem hiding this comment.
Just an oversight, not sure why I chose apple instead of swift 🤔. I can switch it back, though if you think it's not used anywhere I'm happy to just remove it entirely.
There was a problem hiding this comment.
There's a use in lldb/source/Target/Target.cpp:GetScratchTypeSystemForLanguage which is used in lldb/source/Plugins/ExpressionParser/Swift/SwiftREPL.cpp:CreateInstanceFromTarget. So maybe can't remove?
There was a problem hiding this comment.
Eh, you are right, we allow people to pass compiler options to the REPL compiler. For now we need this.
There was a problem hiding this comment.
Okay, thanks for having a look :)! I'll merge this now then and cherry-pick that change to next as well.
There was a problem hiding this comment.
I think we should consistently use SWIFT not APPLE for the swift specific code. After all, this is not specific to APPLE, but to SWIFT...
Other than that this looks fine. If you also fixed TypeSystem.h you could remote the last uses of compiler_options which so far as I can tell never gets used IRL. But the patch is also fine as it is.
There are some Apple-only changes that add an extra parameter to `TypeSystemMap::GetTypeSystemForLanguage`. Add a separate method instead (similar to `TypeSystem::CreateInstance`) and surround the code in `BEGIN/END APPLE` blocks to make it clear they are Apple-only changes. This also fixes compilation failures in `lldb/unittests/Symbol/TestTypeSystem.cpp` since that can now use the regular method instead.
bnbarham
force-pushed
the
more-rebranch-failures
branch
from
bc53247 to
2d3c7da
Compare
|
I've just updated to |
Regenerate bindings static bindings since they need to be updated from the previous PR adding all the missing commits.
Also re-arrange some code to fix a failing test.