Hi Laszlo, Sorry for the delay. I was traveling last week.
I did see this and I have been thinking about it. I think it does make sense to add this new macro for libraries of type BASE. I am surprised we did not run into an issue before that would have required the introduction of this macro earlier. Unless the workaround has been to add #include of <Uefi/UefiBaseTypes.h>, which makes me think we should review BASE libraries to make sure that extra include is not present. The EFI_* error codes are mapped to RETURN_* error codes. So the only feedback I was considering was to implement ASSERT_EFI_ERROR() using ASSERT_RETURN_ERROR(), but that might not always be the right mapping because the RETURN_* codes are a subset of EFI_* error codes. Reviewed-by: Michael Kinney <[email protected]> Best regards, Mike > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Monday, October 24, 2016 2:00 PM > To: Kinney, Michael D <[email protected]>; Gao, Liming > <[email protected]> > Cc: edk2-devel-01 <[email protected]> > Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR() > > Mike, Liming, > > On 10/21/16 23:27, Laszlo Ersek wrote: > > ASSERT_EFI_ERROR() cannot be used in BASE type modules because > > - the replacement text calls EFI_ERROR(), > > - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h", > > - the inclusion of "UefiBaseType.h" is not required for BASE type modules. > > > > While > > > > ASSERT (!RETURN_ERROR (StatusParameter)) > > > > would be a functional statement in BASE type modules, it would be less > > convenient and less informative: ASSERT_EFI_ERROR() prints the actual > > StatusParameter. > > > > Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy the > > original macro definition and update it as follows: > > - replace EFI with RETURN, > > - wrap overlong lines in the comment block and in the code, > > - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead. > > > > Cc: Liming Gao <[email protected]> > > Cc: Michael D Kinney <[email protected]> > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166 > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Laszlo Ersek <[email protected]> > > --- > > > > Notes: > > OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is > > one such BASE module. > > > > MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/MdePkg/Include/Library/DebugLib.h > > b/MdePkg/Include/Library/DebugLib.h > > index 81904325703f..3a910e6a208b 100644 > > --- a/MdePkg/Include/Library/DebugLib.h > > +++ b/MdePkg/Include/Library/DebugLib.h > > @@ -348,6 +348,33 @@ DebugPrintLevelEnabled ( > > #define ASSERT_EFI_ERROR(StatusParameter) > > #endif > > > > +/** > > + Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error > > code. > > + > > + If MDEPKG_NDEBUG is not defined and the > > DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED > > + bit of PcdDebugProperyMask is set, then this macro evaluates the > > + RETURN_STATUS value specified by StatusParameter. If StatusParameter is > > an > > + error code, then DebugAssert() is called passing in the source filename, > > + source line number, and StatusParameter. > > + > > + @param StatusParameter RETURN_STATUS value to evaluate. > > + > > +**/ > > +#if !defined(MDEPKG_NDEBUG) > > + #define ASSERT_RETURN_ERROR(StatusParameter) \ > > + do { \ > > + if (DebugAssertEnabled ()) { \ > > + if (RETURN_ERROR (StatusParameter)) { \ > > + DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \ > > + StatusParameter)); \ > > + _ASSERT (!RETURN_ERROR (StatusParameter)); \ > > + } \ > > + } \ > > + } while (FALSE) > > +#else > > + #define ASSERT_RETURN_ERROR(StatusParameter) > > +#endif > > + > > /** > > Macro that calls DebugAssert() if a protocol is already installed in the > > handle database. > > > > can I please get a maintainer review for this patch? The rest of the > series is ready to go, but it depends on this patch. > > Thanks! > Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
