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

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

I know you're doing things the way it's always been done, but I want to start questioning some long-standing practices :)  So I'm not picking on you specifically, but anywhere we can migrate towards something better incrementally, I think we should do so.



================
Comment at: include/lldb/Target/Target.h:1213-1214
+  public:
+    explicit Arch(const ArchSpec &spec);
+    const Arch &operator=(const ArchSpec &spec);
+
----------------
Add a move constructor maybe?


================
Comment at: include/lldb/Target/Target.h:1217
+    const ArchSpec &GetSpec() const { return m_spec; }
+    Architecture *GetPlugin() const { return m_plugin_up.get(); }
+
----------------
Can this return a reference instead of a pointer?


================
Comment at: source/Core/PluginManager.cpp:281-282
+struct ArchitectureInstance {
+  ConstString name;
+  std::string description;
+  PluginManager::ArchitectureCreateInstance create_callback;
----------------
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`?


================
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;
----------------
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?


================
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:21-23
+ConstString ArchitectureArm::GetPluginNameStatic() {
+  return ConstString("arm");
+}
----------------
Again, doesn't make much sense to me why these need to be const-ified.  They're all string literals anyway, this is just introducing unnecessary locking and contention on the global string pool.


================
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.h:26
+
+  void OverrideStopInfo(Thread &thread) override;
+
----------------
Can you forward declare `Thread` in this file?


================
Comment at: source/Target/Target.cpp:90-95
+      m_mutex(), m_arch(target_arch),
+      m_images(this), m_section_load_history(), m_breakpoint_list(false),
+      m_internal_breakpoint_list(true), m_watchpoint_list(), m_process_sp(),
+      m_search_filter_sp(), m_image_search_paths(ImageSearchPathsChanged, this),
+      m_ast_importer_sp(), m_source_manager_ap(), m_stop_hooks(),
+      m_stop_hook_next_id(0), m_valid(true), m_suppress_stop_hooks(false),
----------------
Can probably delete all of these initializations that are just invoking default constructors, and move the rest to inline class member initialization.


https://reviews.llvm.org/D31172