On 11/17/14 04:22, Scott Duplichan wrote: > Thanks Laszlo and Gabriel. I agree this patch is OK. I just wanted > to use it as an opportunity to point out the inconsistent use of > ASSERT in EDK2. In many cases, the code is written as if ASSERT is > an unconditional stop. In a few cases, though, ASSERT is followed > by CpuDeadLoop().
I agree that this is inconsistent and confusing. I'd prefer if we had a macro that checks a predicate, and if it fails, logs a relevant error message, and stops execution (either by CpuDeadLoop() or by raising a CPU exception, similarly to how ASSERT() is configurable right now). Ie. just drop the possibility to compile out the ASSERT(). > Maybe someone needs to propose an unconditional assert. Lol, maybe I just did that. :) > _ASSERT is > close, but it can continue execution depending on PcdDebugPropertyMask. > _ASSERT is also documented as an "Internal worker macro". ASSERT > followed by CpuDeadLoop is close, but it takes two statements and the > logging depends on MDEPKG_NDEBUG and PcdDebugPropertyMask. Plus CpuDeadLoop() is CpuDeadLoop() unconditionally, why someone might want to opt for the exception. > CpuDeadLoop > alone is good if no logging is needed. Agreed -- and we actually do that in ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationDxeHobLib/HobLib.c [SVN r16134], because we can't use DebugLib there due to circular dependencies. Thanks! Laszlo ------------------------------------------------------------------------------ Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from Actuate! Instantly Supercharge Your Business Reports and Dashboards with Interactivity, Sharing, Native Excel Exports, App Integration & more Get technology previously reserved for billion-dollar corporations, FREE http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
