[Lldb-commits] [PATCH] D90325: [lldb][NFC] Refactor getUUID functionality

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for cleaning this up. A couple of comments inline.



================
Comment at: lldb/include/lldb/Utility/UUID.h:41
+  /// Create a UUID from CvRecordPdb70.
+  static UUID fromData(const CvRecordPdb70 *debug_info,
+                       bool swapByteOrder = true);
----------------
I'd name this like `fromCvRecord` or something. Also, replace the pointer by a reference, please.


================
Comment at: lldb/source/Plugins/Process/minidump/MinidumpParser.cpp:70
+    return UUID::fromData(pdb70_uuid,
+                          !GetArchitecture().GetTriple().isOSBinFormatELF());
   } else if (cv_signature == CvSignature::ElfBuildId)
----------------
This is the only place which passes false, right?
If that's true, I think it would be better to drop this argument, and do something special here. That would give as an opportunity to call out the fact that breakpad (used to) put elf build-ids into this field. This is a sufficiently weird thing to merit drawing extra attention to it. Maybe something like:
```
if (isBinFormatELF()) {
  // Older versions of breakpad used to write the (first 16 bytes of) ELF build into this field.
  return UUID::fromOptionalData(pdb70_uuid->Uuid, sizeof(pdb70_uuid->Uuid))
}
return UUID::fromCvRecord(*pdb70_uuid);  
```


================
Comment at: lldb/source/Utility/UUID.cpp:38
 
+UUID UUID::fromData(const UUID::CvRecordPdb70 *debug_info, bool swapByteOrder) {
+  UUID::CvRecordPdb70 swapped;
----------------
You could just take the argument by value, and then byte-swap it in-place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90325