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