Re: 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
toggle quoted message
Show quoted text
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@...>
Sent: Thursday, May 25, 2023 11:01 AM
To: Oliver Smith-Denny <osde@...>
Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>;
Pop, Aaron <aaronpop@...>
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@...> wrote:No, the idea is that current C code can use the
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,
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 testThis is a hacky solution. Either write things in C++, or don't include
static functions and so include the C file in the unit test
file). Perhaps that is an anti-pattern and googletest has
.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 perhapsI think breaking all sorts of users for this sort of "silly" problems
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
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