labath added a comment.
A collection of small remarks, mostly to do with style. Mainly, I'd like to
make the tests more strict about what they accept as reasonable values.
A higher level question: Given that we are already in the `minidump` namespace,
do we want all (most) of these symbols to be prefixed with `Minidump` ?
Thoughts?
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:65
@@ -68,1 +64,3 @@
+ : m_data_sp(data_buf_sp), m_header(header),
+ m_directory_map(std::move(directory_map)) {}
----------------
this will still invoke the copy-constructor of the map. For that to work, your
constructor needs to take a rvalue reference to the map.
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:154
@@ -142,1 +153,3 @@
+ triple.setOS(llvm::Triple::OSType::Linux);
+ arch_spec.SetTriple(triple);
----------------
How hard would it be to actually detect the os properly here?
Since you've already started processing windows minidumps, it would be great if
we could have a GetArchitecture() test for both OSs.
Update: So I see you already have that test, but it does not check the OS part
of the triple. If it's not too hard for some reason, let's set that as a part
of this CL.
================
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()),
----------------
This is not consistent with the consumeObject function, which also drops the
consumed bytes from the buffer.
================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:127
@@ +126,3 @@
+
+llvm::Optional<pid_t>
+MinidumpMiscInfo::GetPid(const MinidumpMiscInfo &misc_info) {
----------------
`pid_t` is a host-specific type. Use `lldb::pid_t`.
================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:148
@@ +147,3 @@
+ llvm::SmallVector<llvm::StringRef, 42> lines;
+ proc_status.proc_status.split(lines, '\n', 42);
+ for (auto line : lines) {
----------------
This can end up adding 43 elements to the vector. It will still work, but
you'll be doubling your memory usage for no reason. If you really want it, make
the vector larger, but as this is not performance critical, maybe you could
just use `SmallVector<StringRef, 0>` ?
================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:223
@@ +222,3 @@
+
+ static llvm::Optional<const MinidumpString>
+ Parse(llvm::ArrayRef<uint8_t> &data);
----------------
MinidumpString is just a wrapper around the std::string. Why not return the
string directly? (Also the "const" there is unnecessary).
================
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;
----------------
this is oddly formatted now.
================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:307
@@ -288,1 +306,3 @@
+
+ static llvm::Optional<pid_t> GetPid(const MinidumpMiscInfo &misc_info);
};
----------------
It looks like this should be a normal method instead of a static one.
================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:318
@@ +317,3 @@
+
+ static llvm::Optional<pid_t> GetPid(const LinuxProcStatus &misc_info);
+};
----------------
It looks like this should be a normal method instead of a static one.
================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:95
@@ +94,3 @@
+ llvm::Optional<LinuxProcStatus> proc_status = parser->GetLinuxProcStatus();
+ ASSERT_TRUE(proc_status.hasValue());
+}
----------------
Also check whether the returned ProcStatus object has the contents you expect?
================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:111
@@ +110,3 @@
+ ASSERT_EQ(8UL, modules->size());
+ // TODO check for specific modules here
+}
----------------
Fix the todo.
================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:118
@@ +117,3 @@
+ parser->GetStream(MinidumpStreamType::Exception);
+ ASSERT_TRUE(data.hasValue());
+}
----------------
Check the contents of the returned object.
================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:138
@@ +137,3 @@
+ const MinidumpMiscInfo *misc_info = parser->GetMiscInfo();
+ ASSERT_TRUE(misc_info != nullptr);
+}
----------------
Check MiscInfo contents.
https://reviews.llvm.org/D24385
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
