Re: [edk2-devel] GoogleTest Compatibility with MdePkg's IndustyStandard header files

["Oliver Smith-Denny" <osde@linux.microsoft.com>]

Hi Mike,

Thanks for looking for solutions here. This one feels like
quite a back bend, I'm imagining reading code and coming
across TpmStruct.CPLUSPLUS_OPERATOR_KEYWORD and having to
dig around quite a lot to see what goodness is going
on. Because we would have to update the C files, too, right,
depending on the test (there exist tests that want to test
static functions and so include the C file in the unit test
file). Perhaps that is an anti-pattern and googletest has
other ways of testing static functions, but in general it
feels odd to change our functional C code for a C++ unit
test framework.

But, that being said, this is an issue we face, so perhaps
it would be simpler to just rename the members to not conflict
with the C++ keywords, as previously suggested, even though
this may differ from the spec, but it would more align with
EDKII's conventions (shouty case) where the C++ keywords seem
to be lowercase. With the below patch, we would already be
changing the header file to not match with the spec, so from
a readability perspective of the header file, I think
Operator is much more readable than CPLUSPLUS_OPERATOR_KEYWORD.

Thanks,
Oliver

On 5/25/2023 10:06 AM, Michael D Kinney wrote:
> Hi Pedro,
> 
> Thanks for the feedback!
> 
> Applying your pattern to edk2, we find all the usage of c++
> reserved keywords and replace with a macro.  We then define
> that macro to either be the actual c++ reserved keyword if
> build with a C compiler and rename the keyword if building
> with a c++ compiler.  The following patches build with VS
> and GCC and should be no changes from any FW consumers of
> Tpm12.h or Tpm20.h files and should support GoogleTest builds
> of unit test case and code under test that will consistently
> rename the reserved keyword.
> 
> We will see if this resolves Aaron specific use case and if
> it does we can move this to a code review.
> 
> We can always expand the keyword set as needed in the future
> if industry standard specs use C definitions that collide
> with additional c++ reserved keywords.
> 
> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
> index 6597e441a6e2..b98ff33002b8 100644
> --- a/MdePkg/Include/Base.h
> +++ b/MdePkg/Include/Base.h
> @@ -15,6 +15,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   #ifndef __BASE_H__
>   #define __BASE_H__
> 
> +//
> +// C++ reserved keyword defines
> +//
> +#if defined (__cplusplus)
> +#define CPLUSPLUS_OPERATOR_KEYWORD  operator_
> +#define CPLUSPLUS_XOR_KEYWORD       xor_
> +#else
> +#define CPLUSPLUS_OPERATOR_KEYWORD  operator
> +#define CPLUSPLUS_XOR_KEYWORD       xor
> +#endif
> +
>   //
>   // Include processor specific binding
>   //
> diff --git a/MdePkg/Include/IndustryStandard/Tpm12.h b/MdePkg/Include/IndustryStandard/Tpm12.h
> index 155dcc9f5f99..8551dd7c5f42 100644
> --- a/MdePkg/Include/IndustryStandard/Tpm12.h
> +++ b/MdePkg/Include/IndustryStandard/Tpm12.h
> @@ -744,7 +744,7 @@ typedef struct tdTPM_PERMANENT_FLAGS {
>     BOOLEAN              TPMpost;
>     BOOLEAN              TPMpostLock;
>     BOOLEAN              FIPS;
> -  BOOLEAN                           operator;
> +  BOOLEAN                           CPLUSPLUS_OPERATOR_KEYWORD;
>     BOOLEAN                           enableRevokeEK;
>     BOOLEAN              nvLocked;
>     BOOLEAN              readSRKPub;
> diff --git a/MdePkg/Include/IndustryStandard/Tpm20.h b/MdePkg/Include/IndustryStandard/Tpm20.h
> index 4440f3769f26..a96af9cfed63 100644
> --- a/MdePkg/Include/IndustryStandard/Tpm20.h
> +++ b/MdePkg/Include/IndustryStandard/Tpm20.h
> @@ -1247,7 +1247,7 @@ typedef union {
>     TPMI_AES_KEY_BITS    aes;
>     TPMI_SM4_KEY_BITS    SM4;
>     TPM_KEY_BITS         sym;
> -  TPMI_ALG_HASH     xor;
> +  TPMI_ALG_HASH     CPLUSPLUS_XOR_KEYWORD;
>   } TPMU_SYM_KEY_BITS;
> 
>   // Table 123 - TPMU_SYM_MODE Union
> @@ -1320,7 +1320,7 @@ typedef struct {
>   // Table 136 - TPMU_SCHEME_KEYEDHASH Union
>   typedef union {
>     TPMS_SCHEME_HMAC    hmac;
> -  TPMS_SCHEME_XOR  xor;
> +  TPMS_SCHEME_XOR  CPLUSPLUS_XOR_KEYWORD;
>   } TPMU_SCHEME_KEYEDHASH;
> 
>   // Table 137 - TPMT_KEYEDHASH_SCHEME Structure
> 
> 
> Mike
> 
>> -----Original Message-----
>> From: Pedro Falcato <pedro.falcato@gmail.com>
>> Sent: Thursday, May 25, 2023 2:44 AM
>> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Pop, Aaron <aaronpop@microsoft.com>
>> Subject: Re: [edk2-devel] GoogleTest Compatibility with MdePkg's
>> IndustyStandard header files
>>
>> On Thu, May 25, 2023 at 1:24 AM Michael D Kinney
>> <michael.d.kinney@intel.com> wrote:
>>>
>>> That is exactly what I did.  Along with pragma to disable error on macros
>> redefining operators.
>>>
>>> With that change use of "operator" did not generate a warning or error and
>> the build completed.
>>>
>>> Did not work for "xor", and can not find any additional pragma to suppress
>> that error.
>>
>> FWIW, it does work here: https://godbolt.org/z/EEdh9oh53
>>
>> --
>> Pedro
> 
> 
> 
> 
>