Ard, The reason for the current ordering is for size optimization.
The most common implementation of DebugAssertEnabled() uses a FixedAtBuild PCD to determine if these are enabled. The check of status can be optimized away if they are disabled. If you reverse them, then the status check is always performed. Mike > -----Original Message----- > From: Ard Biesheuvel [mailto:[email protected]] > Sent: Thursday, December 7, 2017 7:26 AM > To: [email protected] > Cc: Leif Lindholm <[email protected]>; Gao, Liming > <[email protected]>; Evan Lloyd <[email protected]>; > Kinney, Michael D <[email protected]>; Alexei > Fedorov <[email protected]>; Ard Biesheuvel > <[email protected]> > Subject: Re: [PATCH] MdePkg/DebugLib; swap if conditions > in ASSERT_[EFI|RETURN]_ERROR > > On 7 December 2017 at 15:12, Ard Biesheuvel > <[email protected]> wrote: > > ASSERT_EFI_ERROR () is currently defined as > > > > #if !defined(MDEPKG_NDEBUG) > > #define ASSERT_EFI_ERROR(StatusParameter) > \ > > do { > \ > > if (DebugAssertEnabled ()) { > \ > > if (EFI_ERROR (StatusParameter)) { > \ > > DEBUG ((EFI_D_ERROR, "\nASSERT_EFI_ERROR > (Status = %r)\n", StatusParameter)); \ > > _ASSERT (!EFI_ERROR (StatusParameter)); > \ > > } > \ > > } > \ > > } while (FALSE) > > #else > > #define ASSERT_EFI_ERROR(StatusParameter) > > #endif > > > > This is suboptimal, given that the DebugAssertEnabled > () call in the > > outer if must be executed unconditionally, since the > compiler does not > > know that it does not have any side effects. Instead, > let's swap the > > two ifs, and only call DebugAssertEnabled () if > StatusParameter contains > > an error value to begin with. Do the same for > ASSERT_RETURN_ERROR > > as well. > > > > I just noticed we could do the same for ASSERT () as > well. > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel > <[email protected]> > > Reported-by: Alexei Fedorov <[email protected]> > > --- > > MdePkg/Include/Library/DebugLib.h | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/MdePkg/Include/Library/DebugLib.h > b/MdePkg/Include/Library/DebugLib.h > > index 3a910e6a208b..8369c378e79c 100644 > > --- a/MdePkg/Include/Library/DebugLib.h > > +++ b/MdePkg/Include/Library/DebugLib.h > > @@ -337,8 +337,8 @@ DebugPrintLevelEnabled ( > > #if !defined(MDEPKG_NDEBUG) > > #define ASSERT_EFI_ERROR(StatusParameter) > \ > > do { > \ > > - if (DebugAssertEnabled ()) { > \ > > - if (EFI_ERROR (StatusParameter)) { > \ > > + if (EFI_ERROR (StatusParameter)) { > \ > > + if (DebugAssertEnabled ()) { > \ > > DEBUG ((EFI_D_ERROR, "\nASSERT_EFI_ERROR > (Status = %r)\n", StatusParameter)); \ > > _ASSERT (!EFI_ERROR (StatusParameter)); > \ > > } > \ > > @@ -363,8 +363,8 @@ DebugPrintLevelEnabled ( > > #if !defined(MDEPKG_NDEBUG) > > #define ASSERT_RETURN_ERROR(StatusParameter) > \ > > do { > \ > > - if (DebugAssertEnabled ()) { > \ > > - if (RETURN_ERROR (StatusParameter)) { > \ > > + if (RETURN_ERROR (StatusParameter)) { > \ > > + if (DebugAssertEnabled ()) { > \ > > DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR > (Status = %r)\n", \ > > StatusParameter)); > \ > > _ASSERT (!RETURN_ERROR (StatusParameter)); > \ > > -- > > 2.11.0 > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
