Page MenuHomePhabricator

Factor out common code from the iPhone/AppleTV/WatchOS simulator platform plugins. (NFC)
ClosedPublic

Authored by aprantl on Aug 4 2020, 1:55 PM.

Details

Summary

The implementation of these classes was copied & pasted from the iPhone simulator plugin with only a handful of configuration parameters substituted. This patch moves the redundant implementations into the base class PlatformAppleSimulator.

Diff Detail

Event Timeline

aprantl created this revision.Aug 4 2020, 1:55 PM
aprantl requested review of this revision.Aug 4 2020, 1:55 PM
vsk added inline comments.Aug 4 2020, 3:31 PM
lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
312

s/it$/if$/

557

Can we get into a bad state here when initializing lldb on an embedded device?

aprantl added inline comments.Aug 5 2020, 3:20 PM
lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
557

Good question. I think the pragmatic answer here is that we won't encounter simulator binaries on an embedded device and that the platform will refuse to launch a simulator if one doesn't exist.

aprantl updated this revision to Diff 283413.Aug 5 2020, 3:21 PM
aprantl retitled this revision from [WIP] Factor out common code from the iPhone/AppleTV/WatchOS simulator platform plugins. (NFC) to Factor out common code from the iPhone/AppleTV/WatchOS simulator platform plugins. (NFC).

Update review feedback. This is now good to go (i.e., review). Removing the [WIP] tag.

aprantl updated this revision to Diff 283415.Aug 5 2020, 3:21 PM

Actually update the right patch.

JDevlieghere added inline comments.
lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
85

If you use << you could drop the .str().c_str(). Alternatively, you could use Format which uses the llvm::fmt logic.

lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h
93

"While you are here" could we make these StringRefs?

119

Weird formatting. Did you clang-format?

aprantl added inline comments.Aug 6 2020, 12:52 PM
lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
85

that makes sense, yes.

lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h
93

I could, but these are used specifically/only for the lldb Plugin API:

const char *GetDescription() override { return m_description; }

Do you think it's worth doing regardless?

JDevlieghere added inline comments.Aug 6 2020, 12:55 PM
lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h
93

I think it'd be nice, but more something for a separate NFC patch. It's definitely not that important and I'm sure you have better stuff to do :p

vsk added a comment.Aug 6 2020, 1:22 PM

I gave this another look-over, and while I didn't spot anything troubling, I'm not terribly familiar with this code. I'll give this a third pass later today if review is still needed.

lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
557

All the same, just in case we need to introduce mac-specific code in the future, it may be more convenient to write this as #if TARGET_OS_MAC == 1 && TARGET_CPU_ARM == 1 (hope I've spelled that correctly!)

aprantl added inline comments.Aug 6 2020, 1:38 PM
lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
557

Thanks! In case you didn't already notice, I just wanted to point out that I merely moved this code from one file to another in this commit. That said, that's not an excuse to not improve it, of course!

aprantl added inline comments.Aug 6 2020, 1:50 PM
lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h
93

I just tried to do this, which made me realize that a StringRef is objectively worse. A StringRef doesn't guarantee a NUL-terminated string, so we'd either need to make this a documented requirement (kind of dangerous) or write return m_description.str().c_str(), which is also silly.

The right thing to do is to change the Plugin interface itself to use StringRef.

aprantl updated this revision to Diff 283729.Aug 6 2020, 1:51 PM

Address feedback from @JDevlieghere

aprantl updated this revision to Diff 283730.Aug 6 2020, 1:52 PM
aprantl marked 2 inline comments as done.

clang-format

aprantl marked 4 inline comments as done.Aug 6 2020, 1:54 PM
vsk accepted this revision.Aug 6 2020, 3:39 PM

Looks good to me.

This revision is now accepted and ready to land.Aug 6 2020, 3:39 PM
JDevlieghere accepted this revision.Aug 6 2020, 3:46 PM

Ship it

Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2020, 4:37 PM