GoogleTest Compatibility with MdePkg's IndustyStandard header files

GoogleTest Compatibility with MdePkg's IndustyStandard header files

[aaronpop]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

Google Test, and CPP, has more keywords  C uses.

Tpm12.h and Tpm20.h have references to struct names that are `operator` and `xor`, both of which trigger build errors because they conflict with CPP's keywords.

Operator triggered a build error in MSVC. Xor only triggered a build error under GCC, MSVC did not have a problem with it.

The work arounds suggested in the call, (using defines to get around the conflict) worked for operator, but did not work for xor with gcc.


Tpm12.h:
TPM_PERMANENT_FLAGS
  BOOLEAN                           operator;


Tpm20.h:
TPMU_SCHEME_KEYEDHASH
  TPMS_SCHEME_XOR  xor;

TPMU_SYM_KEY_BITS
  TPMI_ALG_HASH     xor;


What is the suggested method of trying to make existing header files compatible with google test?

Thanks,
Aaron

[-- Attachment #2: Type: text/html, Size: 3401 bytes --]

Re: GoogleTest Compatibility with MdePkg's IndustyStandard header files

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 1736 bytes --]

Hi Aaron,

Don't know if this will completely resolve your issues, but if you add some preprocessor statements around the problematic includes in your unit test CPP file, you may be able to get it to build.

For example, I added the highlighted lines to MdePkg\Test\GoogleTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.cpp and I can get that unit test to build.  Without the #define/#undef lines it fails in the way you describe.

#include <gtest/gtest.h>
extern "C" {
  #include <Base.h>
  #include <Library/SafeIntLib.h>

#define operator operator_
#define xor xor_

  #include <IndustryStandard/Tpm20.h>
  #include <IndustryStandard/Tpm12.h>

#undef operator
#undef xor
}

Mike


From: Aaron Pop <aaronpop@microsoft.com>
Sent: Monday, May 22, 2023 5:41 PM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: GoogleTest Compatibility with MdePkg's IndustyStandard header files

Google Test, and CPP, has more keywords  C uses.

Tpm12.h and Tpm20.h
have references to struct names that are `operator` and `xor`, both of which trigger build errors because they conflict with CPP's keywords.

Operator triggered a build error in MSVC. Xor only triggered a build error under GCC, MSVC did not have a problem with it.

The work arounds suggested in the call, (using defines to get around the conflict) worked for operator, but did not work for xor with gcc.


Tpm12.h:
TPM_PERMANENT_FLAGS
  BOOLEAN                           operator;


Tpm20.h:
TPMU_SCHEME_KEYEDHASH
  TPMS_SCHEME_XOR  xor;
TPMU_SYM_KEY_BITS
  TPMI_ALG_HASH     xor;


What is the suggested method of trying to make existing header files compatible with google test?

Thanks,
Aaron

[-- Attachment #2: Type: text/html, Size: 6281 bytes --]

Re: GoogleTest Compatibility with MdePkg's IndustyStandard header files

[Aaron Pop]

[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]

Hi Mike,

What you suggested does work for MSVC, but it is failing with GCC. It sems that GCC is very strict about operator names.

Relevant errors below:

error: "xor" cannot be used as a macro name as it is an operator in C++
#define xor       XOR
               ^~~
error: "xor" cannot be used as a macro name as it is an operator in C++
#undef xor
MdePkg/Include/IndustryStandard/Tpm20.h:1250:21: error: expected unqualified-id before 'xor' token
TPMI_ALG_HASH     xor;
                                    ^~~
MdePkg/Include/IndustryStandard/Tpm20.h:1323:20: error: expected unqualified-id before 'xor' token
TPMS_SCHEME_XOR  xor;
                                       ^~~


Aaron

From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Wednesday, May 24, 2023 12:49 PM
To: Aaron Pop <aaronpop@microsoft.com>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [EXTERNAL] RE: GoogleTest Compatibility with MdePkg's IndustyStandard header files

Hi Aaron,

Don't know if this will completely resolve your issues, but if you add some preprocessor statements around the problematic includes in your unit test CPP file, you may be able to get it to build.

For example, I added the highlighted lines to MdePkg\Test\GoogleTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.cpp and I can get that unit test to build.  Without the #define/#undef lines it fails in the way you describe.

#include <gtest/gtest.h>
extern "C" {
  #include <Base.h>
  #include <Library/SafeIntLib.h>

#define operator operator_
#define xor xor_

  #include <IndustryStandard/Tpm20.h>
  #include <IndustryStandard/Tpm12.h>

#undef operator
#undef xor
}

Mike


From: Aaron Pop <aaronpop@microsoft.com<mailto:aaronpop@microsoft.com>>
Sent: Monday, May 22, 2023 5:41 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Subject: GoogleTest Compatibility with MdePkg's IndustyStandard header files

Google Test, and CPP, has more keywords  C uses.

Tpm12.h and Tpm20.h
have references to struct names that are `operator` and `xor`, both of which trigger build errors because they conflict with CPP's keywords.

Operator triggered a build error in MSVC. Xor only triggered a build error under GCC, MSVC did not have a problem with it.

The work arounds suggested in the call, (using defines to get around the conflict) worked for operator, but did not work for xor with gcc.


Tpm12.h:
TPM_PERMANENT_FLAGS
  BOOLEAN                           operator;


Tpm20.h:
TPMU_SCHEME_KEYEDHASH
  TPMS_SCHEME_XOR  xor;
TPMU_SYM_KEY_BITS
  TPMI_ALG_HASH     xor;


What is the suggested method of trying to make existing header files compatible with google test?

Thanks,
Aaron

[-- Attachment #2: Type: text/html, Size: 9210 bytes --]

Re: GoogleTest Compatibility with MdePkg's IndustyStandard header files

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 3504 bytes --]

After trying a few GCC experiments, there does not appear to be any way to work around "xor" keyword.

I recommend we update EDK II sources to not use c++ keywords to avoid this issue all together.

This may require changes that do not match names from industry standard specs.

Mike

From: Aaron Pop <aaronpop@microsoft.com>
Sent: Wednesday, May 24, 2023 1:08 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
Subject: RE: GoogleTest Compatibility with MdePkg's IndustyStandard header files

Hi Mike,

What you suggested does work for MSVC, but it is failing with GCC. It sems that GCC is very strict about operator names.

Relevant errors below:

error: "xor" cannot be used as a macro name as it is an operator in C++
#define xor       XOR
               ^~~
error: "xor" cannot be used as a macro name as it is an operator in C++
#undef xor
MdePkg/Include/IndustryStandard/Tpm20.h:1250:21: error: expected unqualified-id before 'xor' token
TPMI_ALG_HASH     xor;
                                    ^~~
MdePkg/Include/IndustryStandard/Tpm20.h:1323:20: error: expected unqualified-id before 'xor' token
TPMS_SCHEME_XOR  xor;
                                       ^~~


Aaron

From: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Sent: Wednesday, May 24, 2023 12:49 PM
To: Aaron Pop <aaronpop@microsoft.com<mailto:aaronpop@microsoft.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Subject: [EXTERNAL] RE: GoogleTest Compatibility with MdePkg's IndustyStandard header files

Hi Aaron,

Don't know if this will completely resolve your issues, but if you add some preprocessor statements around the problematic includes in your unit test CPP file, you may be able to get it to build.

For example, I added the highlighted lines to MdePkg\Test\GoogleTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.cpp and I can get that unit test to build.  Without the #define/#undef lines it fails in the way you describe.

#include <gtest/gtest.h>
extern "C" {
  #include <Base.h>
  #include <Library/SafeIntLib.h>

#define operator operator_
#define xor xor_

  #include <IndustryStandard/Tpm20.h>
  #include <IndustryStandard/Tpm12.h>

#undef operator
#undef xor
}

Mike


From: Aaron Pop <aaronpop@microsoft.com<mailto:aaronpop@microsoft.com>>
Sent: Monday, May 22, 2023 5:41 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Subject: GoogleTest Compatibility with MdePkg's IndustyStandard header files

Google Test, and CPP, has more keywords  C uses.

Tpm12.h and Tpm20.h
have references to struct names that are `operator` and `xor`, both of which trigger build errors because they conflict with CPP's keywords.

Operator triggered a build error in MSVC. Xor only triggered a build error under GCC, MSVC did not have a problem with it.

The work arounds suggested in the call, (using defines to get around the conflict) worked for operator, but did not work for xor with gcc.


Tpm12.h:
TPM_PERMANENT_FLAGS
  BOOLEAN                           operator;


Tpm20.h:
TPMU_SCHEME_KEYEDHASH
  TPMS_SCHEME_XOR  xor;
TPMU_SYM_KEY_BITS
  TPMI_ALG_HASH     xor;


What is the suggested method of trying to make existing header files compatible with google test?

Thanks,
Aaron

[-- Attachment #2: Type: text/html, Size: 10636 bytes --]

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

[Pedro Falcato]

On Wed, May 24, 2023 at 10:23 PM Michael D Kinney
<michael.d.kinney@intel.com> wrote:
>
> After trying a few GCC experiments, there does not appear to be any way to work around “xor” keyword.
>
>
>
> I recommend we update EDK II sources to not use c++ keywords to avoid this issue all together.
>
>
>
> This may require changes that do not match names from industry standard specs.

This is a crappy problem but it's workaroundable in the header by
using something like:

#ifdef __cplusplus
#define SOME_MEMBER_NAME alternative_name
#else
#define SOME_MEMBER_NAME bad_name
#endif

and then in the struct...

struct Foo
{
    UINTN SOME_MEMBER_NAME;
};


It should work around these C++ issues while keeping compat with existing code.
Although yes, avoiding C++ keywords is a good idea, particularly if
you're planning to bring more C++ into edk2.

-- 
Pedro

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

[Michael D Kinney]

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.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pedro
> Falcato
> Sent: Wednesday, May 24, 2023 2:55 PM
> 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 Wed, May 24, 2023 at 10:23 PM Michael D Kinney
> <michael.d.kinney@intel.com> wrote:
> >
> > After trying a few GCC experiments, there does not appear to be any way
> to work around “xor” keyword.
> >
> >
> >
> > I recommend we update EDK II sources to not use c++ keywords to avoid
> this issue all together.
> >
> >
> >
> > This may require changes that do not match names from industry standard
> specs.
> 
> This is a crappy problem but it's workaroundable in the header by
> using something like:
> 
> #ifdef __cplusplus
> #define SOME_MEMBER_NAME alternative_name
> #else
> #define SOME_MEMBER_NAME bad_name
> #endif
> 
> and then in the struct...
> 
> struct Foo
> {
>     UINTN SOME_MEMBER_NAME;
> };
> 
> 
> It should work around these C++ issues while keeping compat with existing
> code.
> Although yes, avoiding C++ keywords is a good idea, particularly if
> you're planning to bring more C++ into edk2.
> 
> --
> Pedro
> 
> 
> 
> 

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

[Pedro Falcato]

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

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

[Michael D Kinney]

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

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

[Oliver Smith-Denny]

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
> 
> 
> 
> 
> 

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

[Pedro Falcato]

On Thu, May 25, 2023 at 6:43 PM Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> 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,

No, the idea is that current C code can use the
already-existing-and-standard .operator and .xor, and C++ can use
.operator_ and .xor_ (or the macros, although please no?).
But my idea was to leave this as a Tpm.h hack, not in Base.h (new
headers and structs should take C++ into account).

> 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

This is a hacky solution. Either write things in C++, or don't include
.c in .cpp.
C code is not C++ code. There's a lot of C code that does not and
should not compile in C++.

So:

1) Write the actual functionality code in C++. This is not yet
supported in EDK2 (I'm a proponent of this)
2) Don't make the functions you're testing static, or make them
conditionally static on something

Note: Adding proper, actual C++ code to EDK2 requires some care, but
could result in actual good changes. I don't know how well this would
be received by the community though.

> 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

I think breaking all sorts of users for this sort of "silly" problems
is not a good option here. There's no actual need for this ATM, apart
from "We want to test this silly code in this new Google Test library
that just appeared upstream".

But these are just my 2c of course.

-- 
Pedro

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

[Michael D Kinney]

Pedro and Oliver,

Yes.  Renaming the struct members is my preferred solution.
This is why I did not send this as a code review as an 
official change request.

It was just to complete the set of options to consider

* No code changes.  Figure out compiler flags to address.
  STATUS: No complete solution found for all compilers.
* Use C-Preprocessor to rename keywords.
  STATUS: Functional, but very bad style and hard to read
  and maintain.
* Rename C structure fields that collide with C++ keywords
  STATUS: Functional.  May break downstream consumers that
  have FW code that references those fields.

Thanks,

Mike

> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Thursday, May 25, 2023 11:01 AM
> To: Oliver Smith-Denny <osde@linux.microsoft.com>
> Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>;
> Pop, Aaron <aaronpop@microsoft.com>
> Subject: Re: [edk2-devel] GoogleTest Compatibility with MdePkg's
> IndustyStandard header files
> 
> On Thu, May 25, 2023 at 6:43 PM Oliver Smith-Denny
> <osde@linux.microsoft.com> wrote:
> >
> > 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,
> 
> No, the idea is that current C code can use the
> already-existing-and-standard .operator and .xor, and C++ can use
> .operator_ and .xor_ (or the macros, although please no?).
> But my idea was to leave this as a Tpm.h hack, not in Base.h (new
> headers and structs should take C++ into account).
> 
> > 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
> 
> This is a hacky solution. Either write things in C++, or don't include
> .c in .cpp.
> C code is not C++ code. There's a lot of C code that does not and
> should not compile in C++.
> 
> So:
> 
> 1) Write the actual functionality code in C++. This is not yet
> supported in EDK2 (I'm a proponent of this)
> 2) Don't make the functions you're testing static, or make them
> conditionally static on something
> 
> Note: Adding proper, actual C++ code to EDK2 requires some care, but
> could result in actual good changes. I don't know how well this would
> be received by the community though.
> 
> > 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
> 
> I think breaking all sorts of users for this sort of "silly" problems
> is not a good option here. There's no actual need for this ATM, apart
> from "We want to test this silly code in this new Google Test library
> that just appeared upstream".
> 
> But these are just my 2c of course.
> 
> --
> Pedro