[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added inline comments.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:23
 #include <map>
+#include <unordered_set>
+#include <vector>
----------------
Why add `<unordered_set>`?  It looks like your new map is just a vector.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:367
   // appropriate range linearly each time is stupid.  Perhaps we should build
-  // an index for faster lookups.
+  // an i for faster lookups.
   llvm::Optional<minidump::Range> range = FindMemoryRange(addr);
----------------
This looks like an inadvertent change.  Please change "i" back to "index".


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:454
+Status MinidumpParser::Initialize() {
+  Status result;
+
----------------
For consistency, please consider renaming `result` to `error`.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:558
+  return result;
+}
----------------
This is a long function (which, unfortunately, is not unusual in LLVM/LLDB), but the comments here are really good, so I'm OK with it.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:93
+  // Perform consistency checks and initialize internal data structures
+  Status Initialize();
+
----------------
I'm not a big fan of 2-step initialization, but that seems to be the way of LLDB. :-(


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:172
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();
----------------
Should the architecture check be in the MinidumpParser::Initialize with the other checks?

I don't know the answer; I'm just asking for your thinking about this.


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:193
   if (!pid) {
-    error.SetErrorString("failed to parse PID");
+    error.SetErrorString("Failed to parse PID");
     return error;
----------------
I realize nothing is perfectly consistent, but I think LLVM tends to start error strings with a lowercase letter (except for proper nouns).  Can you revert this case change and make sure your new error strings follow that pattern?


================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
     ASSERT_GT(parser->GetData().size(), 0UL);
+    auto result = parser->Initialize();
+    if (expect_failure)
----------------
I would rather see this function return the result of the Initialize and let the individual tests check the expectation explicitly.

I know that will lead to a little bit of duplication in the tests, but it will make the individual tests easier to understand in isolation.  It also makes it possible for each test to decide whether to ASSERT or EXPECT.  And it eliminates the need for the bool parameter which is hard to decipher at the call sites.


================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:80
+  SetUpData("linux-x86_64.dmp", 100, true);
+  SetUpData("linux-x86_64.dmp", 20 * 1024, true);
 }
----------------
See comment on SetUpData.  As much as practical, I'd rather have the EXPECTs and ASSERTs directly in the tests rather than hiding them in a helper function.  Also note that, while the first two arguments are pretty intuitive, there's no way to understand the true/false in the third argument without going to look up to see what SetUpData does.


https://reviews.llvm.org/D49202