Re: [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 7 December 2017 at 17:01, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
> 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.
>

DebugAssertEnabled() is a function call that gets resolved at link
time, and is not annotated as being free of side effects. So I agree
that the implementation of that function could be optimized into a
'return true' or 'return false' depending on the compile time values
of those PCDs, but the way the macro is defined currently, it still
requires the function call to be made, and the conditional compare
with a constant that follows will still be present in the code.

What I am suggesting is to replace it with a comparison with a
constant, and a conditional function call instead. This will not
affect code size, but will only remove needless function calls at
runtime.