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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
Update review feedback. This is now good to go (i.e., review). Removing the [WIP] tag.
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? |
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? |
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 |
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!) |
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! |
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. |
"While you are here" could we make these StringRefs?