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

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

LGTM if you don't want to consider my remaining suggestion for this patch.

Thanks for the extra tests.



================
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:
> > 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.
> So how would we handle the existing checks in SetupData()?
SetUpData would just return an error status instead of ASSERTing.  The actual ASSERTs would be placed in the tests that call SetUpData.  As I said, that would add some duplication, because individual tests would have to ASSERT (or EXPECT) on the result of SetUpData, but it makes the tests easier to read and maintain the tests.

Since there are other tests using SetUpData, they would have to be updated, so maybe you want to declare this suggestion as out-of-scope for this patch.  Anyway, I'm happy that you eliminated the boolean argument.


https://reviews.llvm.org/D49202