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

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: source/Core/PluginManager.cpp:281-282
+struct ArchitectureInstance {
+  ConstString name;
+  std::string description;
+  PluginManager::ArchitectureCreateInstance create_callback;
----------------
labath wrote:
> 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).
I don't think we need to enforce it.  Documentation is good enough.  If someone accepts a `StringRef` to a function, you, as the caller of the function, cannot enforce what they do with it.  They could store a pointer to it.  That doesn't mean you should never pass in a `std::string` and let the implicit conversion happen, it just means you have to follow the contract.  Same could happen if your function took a `const std::string&`. 
 Documenting the top level interface to say "The  memory for this string is assumed to live for the life of the program" should be sufficient.

If someone needs to do something funky and construct a name dynamically, they can make their plugin contain an `llvm::StringSaver` and get a stable pointer that way.


================
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;
----------------
labath wrote:
> 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.
So, follow up question...  Can we remove it?  


https://reviews.llvm.org/D31172