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

[Greg Clayton via Phabricator via lldb-commits]

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