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