Re: [edk2-devel] [PATCH v6 14/22] ArmPkg: Add Universal/Smbios/ProcessorSubClassDxe

["Laszlo Ersek" <lersek@redhat.com>]

On 01/28/21 12:32, Leif Lindholm wrote:
> On Wed, Jan 27, 2021 at 21:53:57 -0700, Rebecca Cran wrote:
>> On 1/25/21 12:04 PM, Leif Lindholm wrote:
>>> On Thu, Jan 14, 2021 at 09:36:20 -0700, Rebecca Cran wrote:
>>>> +// Sets the HII variable `x` if `pcd` isn't empty
>>>> +#define SET_HII_STRING_IF_PCD_NOT_EMPTY(pcd, x)               \
>>>> +    x##Str = (CHAR16 *)PcdGetPtr (pcd); \
>>>> +    if (StrLen (x##Str) > 0) {                                \
>>>> +      HiiSetString (mHiiHandle, x, x##Str, NULL);             \
>>>> +    }                                                         \
>>>
>>> I am not a fan of preprocessor macros that require local variables
>>> with magic names to exist.
>>> Can this be rewritten as a helper function?
>>
>> Unfortunately PcdGetPtr uses token pasting, so a helper function won't work.
>>
>> Would the following be better?
>>
>> #define SetHiiStringIfPcdNotEmpty(Pcd, StringId) { \
>>   CHAR16 *Str = (CHAR16*)PcdGetPtr (Pcd); \
>>   if (StrLen (Str) > 0) { \
>>     HiiSetString (mHiiHandle, StringId, Str, NULL); \
>>   } \
>> }
> 
> I'm mostly OK with that. I'd say it technically violates the coding
> style because of assigning a value to Str on definition, but I can't
> convince myself to care about that for a macro.

It wouldn't be hard to fix.

> 
> However, it still needs to have the name indicate that it's a macro,
> so SET_HII_STRING_IF_PCD_NOT_EMPTY.

A "do { ... } while (0)" around the whole thing is missing as well.

Laszlo