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

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Also, I'm not seeing tests for the other consistency checks you're adding (like whether there are any streams or whether the streams overlap).  Is it difficult to create such malformed minidumps?



================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
     ASSERT_GT(parser->GetData().size(), 0UL);
+    auto result = parser->Initialize();
+    if (expect_failure)
----------------
lemo wrote:
> 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)
This isn't quit what I meant.  I'd rather not have the ASSERTs in helper functions at all (regardless of return type).  The helpers should return a status and the actual test code should do whatever ASSERT or EXPECT is appropriate.


https://reviews.llvm.org/D49202