[Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

[Zachary Turner via lldb-commits]

zturner added inline comments.

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:222
@@ -154,1 +221,2 @@
+  return MinidumpExceptionStream::Parse(data);
 }
----------------
dvlahovski wrote:
> Now that I return an ArrayRef<ModuleList>, which is basically a reinterpret cast of a piece of memory to a list of modules, do you think that this is still something good to do?
> 
> At least for now, I'm calling this method only once in the DoLoadCore initializing method of the plugin (still unsubmitted code) and then creating the ModuleSpecs for each module.
Yea I suppose this is fine now.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:21
@@ +20,3 @@
+llvm::StringRef
+lldb_private::minidump::consumeString(llvm::ArrayRef<uint8_t> &Buffer) {
+  auto str = llvm::StringRef(reinterpret_cast<const char *>(Buffer.data()),
----------------
dvlahovski wrote:
> So, breakpad specific string in the Minidump files are not UTF-16 string, but just ascii strings in the file (not null terminated).
> When I get the stream "LinuxProcStatus" I know how big it is and set it in the ArrayRef.
> So basically it is my intention here to parse the entire ArrayRef to a string
In that case it seems like this function is unnecessary.  If all it does is reinterpret an entire `ArrayRef` as a `StringRef`, it seems better to do that at each callsite.  A generic name like `consumeString` makes people think of either zero terminated string or length prefixed strength, and this one is neither.

Another option might be to add an additional length argument to the `consumeString` function, and then call it like `llvm::StringRef S = consumeString(Buffer, Buffer.size())` with a comment about why you're treating the entire thing as a string.


https://reviews.llvm.org/D24385