labath added inline comments.
================
Comment at: include/lldb/Target/Target.h:1217
+ const ArchSpec &GetSpec() const { return m_spec; }
+ Architecture *GetPlugin() const { return m_plugin_up.get(); }
+
----------------
zturner wrote:
> Can this return a reference instead of a pointer?
It can't, because the architecture plugin is not always present (currently we
only have the arm one, and I currently don't see a use for others). We could
consider a no-op plugin, which gets used if no specific plugin is present. This
would even avoid the need for this class, as then the plugin could be
responsible for returning the archspec.
================
Comment at: source/Core/PluginManager.cpp:281-282
+struct ArchitectureInstance {
+ ConstString name;
+ std::string description;
+ PluginManager::ArchitectureCreateInstance create_callback;
----------------
zturner wrote:
> Why do we need a `ConstString` and a `std::string` here? I don't think any
> plugin ever has a dynamically generated name or description, they exclusively
> originate from string literals. So, with that in mind, can both of these
> just be `StringRef`?
They, could be, but I don't see a way to enforce that the names come from
string literals, which makes me wonder if the usage of StringRef is a win in
this case...
IIRC, the main reason I made this ConstString (instead of std::string, like the
description), is that this name eventually makes it's way to the
PluginInterface virtual method (which cannot be changed without touching every
plugin).
================
Comment at: source/Core/PluginManager.cpp:318-323
+ std::lock_guard<std::mutex> guard(g_architecture_mutex);
+ for (const auto &instances : GetArchitectureInstances()) {
+ if (auto plugin_up = instances.create_callback(arch))
+ return plugin_up;
+ }
+ return nullptr;
----------------
zturner wrote:
> These mutexes have always bothered me. Are people really registering and
> unregistering plugins dynamically? It seems to me all of this always happens
> at debugger startup. Are the mutexes necessary?
I'm pretty sure they aren't needed. I was just following what the other plugins
do.
https://reviews.llvm.org/D31172
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
