Re: [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add and update debug ASSERTS

[Leif Lindholm <leif.lindholm@linaro.org>]

On Wed, Mar 14, 2018 at 12:35:03PM +0000, Ard Biesheuvel wrote:
> >> I guess Leif and I are in disagreement here. In particular, I think
> >> his comment
> >>
> >> """
> >> ASSERT (FALSE)?  (You already know Status is an EFI_ERROR, and a
> >> console message saying ASSERT (Status) is not getting you out of
> >> looking at the source code to find out what happened.)
> >> """
> >>
> >> is misguided, given that ASSERT_EFI_ERROR (Status) will actually print
> >> the value of Status to the debug console.
> >
> > I don't have a strong enough opinion on this that it should be
> > listened to if you both agree.
> >
> > It's just the "if error, then assert if error" that breaks up the
> > parsing flow for me. Could it make sense to use ASSERT_RETURN_ERROR
> > instead?
> >
> > So
> >       if (EFI_ERROR (Status)) {
> >         ASSERT_RETURN_ERROR (Status);
> >       }
> >
> 
> Do you mean using ASSERT_RETURN_ERROR() rather than ASSERT_EFI_ERROR()?

Yes.

> That doesn't make sense: the former is for RETURN_STATUS type
> variables and the latter for EFI_STATUS

Right, of course.
Makes the text look nicer though :)

Anyway, I'm OK with the original version.
Just wish we had a less redundant-looking way to describe this.

/
    Leif