lemo added inline comments.
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:23
#include <map>
+#include <unordered_set>
+#include <vector>
----------------
amccarth wrote:
> Why add `<unordered_set>`? It looks like your new map is just a vector.
Good catch
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:558
+ return result;
+}
----------------
amccarth wrote:
> 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.
I agree size-wise it's borderline. It's also a bit smelly in that it does two
things (checks + initialization).
I originally had a separate "ConsistencyCheck", but there was significant
duplication (the directory traversal) and the combined version won by a tiny
margin :) If this grows with more checks I'd be happy to revisit and refactor
it.
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:93
+ // Perform consistency checks and initialize internal data structures
+ Status Initialize();
+
----------------
amccarth wrote:
> I'm not a big fan of 2-step initialization, but that seems to be the way of
> LLDB. :-(
Agreed. I don't see how to avoid this w/o much bigger changes though (we don't
have a chance to return meaningful errors during process creation).
================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:172
+
+ // Do we support the minidump's architecture?
+ ArchSpec arch = GetArchitecture();
----------------
amccarth wrote:
> 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.
Good question, here's my take: the checks are for consistency and a minidump
with a currently unsupported architecture is a valid minidump.
So I think it's better to have this check external since the architecture
support is not a minidump parser concern. WDYT?
================
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;
----------------
amccarth wrote:
> 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?
Sigh, sure. I was hoping it's the other way around.
================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
ASSERT_GT(parser->GetData().size(), 0UL);
+ auto result = parser->Initialize();
+ if (expect_failure)
----------------
amccarth wrote:
> 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.
Good idea, although gunit doesn't let us ASSERT in non-void returning functions.
I agree that the bool parameter is ugly, so I created a separate
TruncateMinidump() helper (which cleans up the SetUpData since the load_size
was only used for truncation)
https://reviews.llvm.org/D49202
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
