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

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Thanks Adrian for the thorough review.



================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
     ASSERT_GT(parser->GetData().size(), 0UL);
+    auto result = parser->Initialize();
+    if (expect_failure)
----------------
amccarth wrote:
> 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.
I think I understand the idea and I agree it's tempting. The problem is that not all the checks in SetupData() are status based, so there's no direct mapping from each operation to a return value. 

It doesn't seem terribly bad to let the helper encapsulate some of the checks.


https://reviews.llvm.org/D49202