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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
