Re: [edk2-devel] [PATCH] MdePkg: DebugLib: Compilation fix for clang-13.

["Marvin Häuser" <mhaeuser@posteo.de>]

23.11.2021 00:17:30 Michael Brown <mcb30@ipxe.org>:

> On 22/11/2021 16:42, Michael D Kinney wrote:
>> You are also modifying the DebugLib in the paths where ASSERT() macros
>> are disabled.  When they are disabled, we want all code/data associated
>> with ASSERT() to be removed by the optimizing compiler/linker.  The source
>> code change appears to force a reference to a variable/expression.  Does
>> this have any size impact to any of the toolchains when ASSERT() is
>> disabled?  Can you provide the size comparison before and after this
>> change?
>
> I would very strongly recommend having the non-debug version of the macro use something like:
>
> #define ASSERT(Expression) do {   \
>      if (FALSE) {                  \
>        (VOID) (Expression);        \
>      }                             \
>    } while (FALSE)
>
> Without the "if (FALSE)", you will find that an expression that may have side-effects (e.g. by calling an external function) will result in executable code being generated.

In theory +1, but don't some compilers generate "unreachable code" warnings for this? I unfortunately cannot check them all right now. Maybe guards push/pop'ing the warning for that need to be defined, that are only allowed to be used with explicit documentation why they are necessary?

Best regards,
Marvin

>
> Michael
>
>
>