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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: include/lldb/Host/UserIDResolver.h:24
+public:
+  typedef uint32_t id_t;
+  virtual ~UserIDResolver(); // anchor
----------------
make this 64 bit for future proofing? And if so, just use lldb::user_id_t?


================
Comment at: include/lldb/Host/UserIDResolver.h:27
+
+  llvm::Optional<llvm::StringRef> GetUserName(id_t Uid) {
+    return Get(Uid, UidCache, &UserIDResolver::DoGetUserName);
----------------
If we are storing this as an optional std::string, why hand it out as a StringRef?


================
Comment at: include/lldb/Host/UserIDResolver.h:49-51
+  std::mutex Mutex;
+  Map UidCache;
+  Map GidCache;
----------------
prefix with "m_", make lower case and separate by "_"


================
Comment at: source/Commands/CommandObjectPlatform.cpp:1154
             if (platform_sp->GetProcessInfo(pid, proc_info)) {
-              ProcessInstanceInfo::DumpTableHeader(ostrm, platform_sp.get(),
-                                                   m_options.show_args,
+              ProcessInstanceInfo::DumpTableHeader(ostrm, m_options.show_args,
                                                    m_options.verbose);
----------------
Why are we not passing the platform in still? What if we need it for something else?


================
Comment at: source/Commands/CommandObjectPlatform.cpp:1156
                                                    m_options.verbose);
-              proc_info.DumpAsTableRow(ostrm, platform_sp.get(),
+              proc_info.DumpAsTableRow(ostrm, platform_sp->GetUserIDResolver(),
                                        m_options.show_args, m_options.verbose);
----------------
Ditto


================
Comment at: source/Host/CMakeLists.txt:23
   common/FileCache.cpp
+  common/File.cpp
   common/FileSystem.cpp
----------------
remove whitespace change unless it is needed?


================
Comment at: source/Host/CMakeLists.txt:37
   common/NativeThreadProtocol.cpp
+  common/NativeWatchpointList.cpp
   common/OptionParser.cpp
----------------
remove whitespace change unless it is needed?


================
Comment at: source/Host/CMakeLists.txt:45
   common/SocketAddress.cpp
+  common/Socket.cpp
   common/StringConvert.cpp
----------------
remove whitespace change unless it is needed?


================
Comment at: source/Target/Process.cpp:281
 
-void ProcessInstanceInfo::Dump(Stream &s, Platform *platform) const {
-  const char *cstr;
+void ProcessInstanceInfo::Dump(Stream &s, UserIDResolver &resolver) const {
   if (m_pid != LLDB_INVALID_PROCESS_ID)
----------------
So we are currently only using the platform for the user ID resolving, but it might be nice to keep the platform as the argument in case we need it for something else in the future?


================
Comment at: source/Target/Process.cpp:352
 
-void ProcessInstanceInfo::DumpAsTableRow(Stream &s, Platform *platform,
+void ProcessInstanceInfo::DumpAsTableRow(Stream &s, UserIDResolver &resolver,
                                          bool show_args, bool verbose) const {
----------------
Ditto


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

https://reviews.llvm.org/D58167