[Lldb-commits] [PATCH] D58167: Refactor user/group name resolving code

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added inline comments.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp:439
   if (uid != UINT32_MAX) {
-    std::string name;
-    if (HostInfo::LookupUserName(uid, name)) {
+    if (auto name = HostInfo::GetUserIDResolver().GetUserName(uid)) {
       StreamString response;
----------------
jingham wrote:
> labath wrote:
> > jingham wrote:
> > > Do we need auto here?  Since we have a bunch of API's returning StringRef's now when I see strings returned I get paranoid about their lifespan.  auto hides the fact that I don't need to worry...
> > I've removed the auto, though I am not sure if that alleviates your fears, as the returned type is StringRef. There is still nothing to worry about though, as the backing storage is held by the resolver object.
> So how do I reason about the lifespan of this StringRef, then?  Now I have to know that GetUserIDResolver doesn't make a temporary UserIDResolver, but a reference to one that persists - for how long again?
Yes, that would be line of reasoning I envisioned. The returned resolver (and, transitively, the storage backing the StringRef) ought to exist at least while the "HostInfo" class is valid, so from HostInfo::Initialize() and until HostInfo::Finalize(). In practice it will be even longer because the resolver is constructed lazily on first use, and will be destroyed only by llvm_shutdown (which we don't call ever), but that's not what I would promise. For the "platform" version of the GetUserIDResolver function, the resolver ought to exist for as long as the platform instance you got it from is alive.

I can put this into the method comments, but it seems pretty straigh-forward to me (e.g. I would expect this comment to apply to all HostInfo functions. I definitely know that HostInfo::GetArchitecture blows up if called before `Initialize()`).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58167/new/

https://reviews.llvm.org/D58167