Re: [PATCH v3 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep.

["Ni, Ray" <ray.ni@Intel.com>]

On 2/23/2019 1:16 AM, Laszlo Ersek wrote:
> Yes, I was fully aware of that.
> 
> However:
> 
> The issue is that, in the BmReportLoadFailure() function, we do some
> work*before*  we call REPORT_STATUS_CODE_EX(). We have an ASSERT(), a
> ZeroMem(), and a field assignment.
> 
> If status code reporting is disabled for EFI_ERROR_CODE in the platform,
> then said work will be wasted. We can optimize this by checking for
> ReportErrorCodeEnabled() up-front, because we know for sure that later
> on we will report the status code with EFI_ERROR_CODE type.
> 
> In other words, this approach is similar to DEBUG_CODE(). In some cases,
> logging a piece of information with DEBUG() takes non-trivial
> computation. And it would be a waste, for example in RELEASE builds, to
> perform the computation, and then throw away only the result (the log
> message). Therefore the DEBUG_CODE macro is used, and the whole work is
> eliminated in RELEASE builds.
> 
> The idea is the same here. If the compiler can statically deduce that
> ReportErrorCodeEnabled() will always return FALSE -- for example because
> the ReportStatusCodeLib instance in question looks at
> "PcdReportStatusCodePropertyMask", and the PCD is Fixed-at-Build, and
> the corresponding bit is clear --, then the compiler can eliminate the
> entire BmReportLoadFailure() function. This is good for both flash usage
> and for performance.
> 
> I'm fine either way, but first, please confirm again that you really
> want me to remove the ReportErrorCodeEnabled() check, before pushing.
> 
> Thanks!
> Laszlo

Thanks for the explanation. I am fine with both.
Reviewed-by: Ray Ni <ray.ni@intel.com>

-- 
Thanks,
Ray