amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.
The big picture here is fine.
I see a couple opportunities in the details, but I won't block on them.
================
Comment at: include/lldb/Core/Module.h:779
+ //------------------------------------------------------------------
+ void SetUUID(const lldb_private::UUID &uuid);
+
----------------
Was there no SetUUID before because the UUID was considered immutable? I
wonder if that's worth keeping. Is the UUID always known at construction time?
================
Comment at: source/Core/Module.cpp:341
+ m_uuid = uuid;
+ m_did_parse_uuid = true;
+}
----------------
I wonder if the lazy UUID lookup in Module::GetUUID should call this to avoid
code duplication. I know it's not a lot of code, but it involves a mutex and
an atomic flag, so it might be nice to always have this update in just one
place.
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:99
+UUID MinidumpParser::GetModuleUUID(const MinidumpModule *module) {
+ auto cv_record =
----------------
I wonder if this should return an `Expected<UUID>`. The function has multiple
ways it can fail, in which case the error info is lost and we silently return
an empty UUID.
https://reviews.llvm.org/D46292
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
