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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: include/lldb/Host/UserIDResolver.h:24
+public:
+  typedef uint32_t id_t;
+  virtual ~UserIDResolver(); // anchor
----------------
clayborg wrote:
> make this 64 bit for future proofing? And if so, just use lldb::user_id_t?
I think the best way to future-proof things is to use a separate type for logically distinct entities. While user_id_t sounds like it would be the right type, it is actually used for a completely different purpose.

And while I don't fully understand how windows user id's work, they seem to be represented as strings of the form "S-1-5-21-7375663-6890924511-1272660413-2944159". If that's the thing we're going to use on windows, then even 64 bits will not be enough, and we may have to use something different altogether.

I used 32-bits for now, because that's what it was used until now, and it seemed to be enough. There are also some latent uses of UINT32_MAX for invalid uids, which would need to be tracked down to change that.


================
Comment at: include/lldb/Host/UserIDResolver.h:27
+
+  llvm::Optional<llvm::StringRef> GetUserName(id_t Uid) {
+    return Get(Uid, UidCache, &UserIDResolver::DoGetUserName);
----------------
clayborg wrote:
> If we are storing this as an optional std::string, why hand it out as a StringRef?
I am storing it as std::string, as I need it to provide storage, but generally, I think the public APIs should use StringRefs for non-owning string references. I could hand this out as `const llvm::Optional<std::string> &`, but that would be even longer to type, and would increase the chance of someone doing an unintentional string copy.


================
Comment at: source/Host/CMakeLists.txt:23
   common/FileCache.cpp
+  common/File.cpp
   common/FileSystem.cpp
----------------
clayborg wrote:
> remove whitespace change unless it is needed?
I've committed the sorting as a separate patch.


================
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:
> 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.


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

https://reviews.llvm.org/D58167