clayborg 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?
No, most architectures don't have one of these plug-ins. It isn't mandatory, so
NULL is just fine.
================
Comment at: source/Core/PluginManager.cpp:281
+struct ArchitectureInstance {
+ ConstString name;
+ std::string description;
----------------
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`?
ConstString makes for faster lookups. Many plugins can ask for a plug-in by
name, so ConstString works well in that regard.
================
Comment at: source/Core/PluginManager.cpp:282
+ ConstString name;
+ std::string description;
+ PluginManager::ArchitectureCreateInstance create_callback;
----------------
We need "std::string" since it owns the storage. Most people do init this with
a const string, but they don't need to. ConstString doesn't work here because
we never will search for a description. StringRef doesn't own the storage, so I
would rather avoid StringRef unless you can guarantee no one can construct a
StringRef with local data.
================
Comment at: source/Core/PluginManager.cpp:323
+ }
+ return nullptr;
+}
----------------
You are probably correct. I don't really mind the thread safety. We would need
to worry about this more if people could write external plug-ins, but that
isn't the case here. I would just leave it personally.
================
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:23
+ return ConstString("arm");
+}
+
----------------
One time at startup. No threads contending yet. Asking for plug-in by name is
made fast for later. I would leave this.
================
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.h:26
+
+ void OverrideStopInfo(Thread &thread) override;
+
----------------
zturner wrote:
> Can you forward declare `Thread` in this file?
use lldb-forward.h
https://reviews.llvm.org/D31172
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
