clayborg added inline comments.
================
Comment at: source/Core/PluginManager.cpp:282
+ ConstString name;
+ std::string description;
+ PluginManager::ArchitectureCreateInstance create_callback;
----------------
zturner wrote:
> 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`
Why not just use a std::string then. Both std::string and StringRef will have
similar compare times.
================
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:23
+ return ConstString("arm");
+}
+
----------------
zturner wrote:
> clayborg wrote:
> > One time at startup. No threads contending yet. Asking for plug-in by name
> > is made fast for later. I would leave this.
> Is asking for a plugin by name something that happens in a hot path?
No. If we are talking about making the code safer, I would rather go with
something that can guarantee safety. The StringRef has the possibility of going
wrong if someone uses a local string. So unless we can guarantee it I don't see
a reason to change.
https://reviews.llvm.org/D31172
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
