zturner added inline comments.
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:88-89
@@ +87,4 @@
+MinidumpParser::GetMinidumpString(uint32_t rva) {
+ llvm::ArrayRef<uint8_t> arr_ref(m_data_sp->GetBytes() + rva,
+ m_data_sp->GetByteSize() - rva);
+ return MinidumpString::Parse(arr_ref);
----------------
This is fine, but I prefer to avoid the math wherever possible since it makes
code slightly more difficult to read. I would write this as:
```
auto arr_ref = m_data_sp->GetData().drop_front(rva);
```
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:171
@@ +170,3 @@
+llvm::Optional<LinuxProcStatus> MinidumpParser::GetLinuxProcStatus() {
+ llvm::Optional<llvm::ArrayRef<uint8_t>> data =
+ GetStream(MinidumpStreamType::LinuxProcStatus);
----------------
Same as before, an `Optional<ArrayRef>` seems a bit redundant. I would just
return an empty `ArrayRef` to represent it not being there. I dont' think
there's a valid case for a strema to actually be present, but be 0 bytes in
length.
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:202
@@ +201,3 @@
+
+ return MinidumpModule::ParseModuleList(data.getValue());
+}
----------------
It will do this parsing every time you ask for the module list. How about
storing a `std::vector<const MinidumpModule *>` in the `MinidumpParser` class
and then you can return an `ArrayRef<const MinidumpModule*>` to it. So it's
lazy evaluation, only done once, with the side benefit of allowing you to
return an `ArrayRef<T>` instead of a more complicated `Optional<vector<T>>`
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:47
@@ -45,1 +46,3 @@
+ llvm::Optional<const MinidumpString> GetMinidumpString(uint32_t rva);
+
----------------
There's a lot of `Optional`'s here. Is it really possible for any subset of
these to be optional? Or is it more like if you find one, you will find them
all?
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:61
@@ +60,3 @@
+
+ llvm::Optional<std::vector<const MinidumpModule *>> GetModuleList();
+
----------------
I would drop the `Optional` here and return `ArrayRef<const MinidumpModule*>`.
Returning an entire vector by value is wasteful on the stack. Using an
`ArrayRef` makes it very lightweight, while still providing value-like
semantics in that you won't be able to modify the underlyign container. Also,
if its size is 0, you can treat that the same as if the `Optional` did not have
a value.
================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:21
@@ +20,3 @@
+llvm::StringRef
+lldb_private::minidump::consumeString(llvm::ArrayRef<uint8_t> &Buffer) {
+ return llvm::StringRef(reinterpret_cast<const char *>(Buffer.data()),
----------------
labath wrote:
> This is not consistent with the consumeObject function, which also drops the
> consumed bytes from the buffer.
Is this logic correct? A buffer may be arbitrarily large and have more data in
it besides the string. Perhaps you need to search forward for a null
terminator, then only return that portion of the string, then drop that many
bytes (plus the null terminator) from the input buffer?
================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:223
@@ +222,3 @@
+
+ static llvm::Optional<const MinidumpString>
+ Parse(llvm::ArrayRef<uint8_t> &data);
----------------
labath wrote:
> MinidumpString is just a wrapper around the std::string. Why not return the
> string directly? (Also the "const" there is unnecessary).
Agree, the `MinidumpString` class seems unnecessary to me. Just make
`parseMinidumpString` be a file static global function and return an
`std::string`.
================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:299
@@ -282,1 +298,3 @@
+ llvm::support::ulittle32_t
+ flags1; // represent what info in the struct is valid
llvm::support::ulittle32_t process_id;
----------------
labath wrote:
> this is oddly formatted now.
Probably because the comment passed 80 columns. I would put the comment on a
new line above the field to fix this.
https://reviews.llvm.org/D24385
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
