[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
+struct ArchitectureInstance {
+  ConstString name;
+  std::string description;
----------------
zturner wrote:
> labath wrote:
> > clayborg 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`?
> > > ConstString makes for faster lookups. Many plugins can ask for a plug-in by name, so ConstString works well in that regard.
> > 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.
I'm not convinced it makes for faster lookups, because while true that it is "just" a pointer comparison, it is a pointer comparison that involves taking a global mutex.

In the case of `StringRef` constructed from a string literal (which as I mentioned, is I believe 100% of usage today), `StringRef` will be faster, because it will pass an unguarded pointer comparison.  Only if the pointer comparisons fail does `StringRef` fall back to `memcmp`  (or at least, it *should* work that way, if it doesn't we can make it)


================
Comment at: source/Core/PluginManager.cpp:282
+  ConstString name;
+  std::string description;
+  PluginManager::ArchitectureCreateInstance create_callback;
----------------
clayborg wrote:
> 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.
But why do you need to own the storage of a string literal?  The binary already owns that storage.  Just document in the API that it needs to be stable storage, since that covers 100% of existing use cases.  And if someone needs something else they can use an `llvm::StringSaver`


https://reviews.llvm.org/D31172