[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process

[Greg Clayton via Phabricator via lldb-commits]

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