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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
