public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Pedro Falcato" <pedro.falcato@gmail.com>
To: Oliver Smith-Denny <osde@linux.microsoft.com>
Cc: devel@edk2.groups.io, michael.d.kinney@intel.com,  "Pop,
	Aaron" <aaronpop@microsoft.com>
Subject: Re: [edk2-devel] GoogleTest Compatibility with MdePkg's IndustyStandard header files
Date: Thu, 25 May 2023 19:01:20 +0100	[thread overview]
Message-ID: <CAKbZUD2q95W7WEdiYqiXSoOt24aAvsOgn+0w34zuMtavMxi6cA@mail.gmail.com> (raw)
In-Reply-To: <f03ddf22-48fd-1b86-8ab0-bf789eeedaa7@linux.microsoft.com>

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

  reply	other threads:[~2023-05-25 18:01 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-23  0:40 GoogleTest Compatibility with MdePkg's IndustyStandard header files aaronpop
2023-05-24 19:49 ` Michael D Kinney
2023-05-24 20:07   ` Aaron Pop
2023-05-24 21:23     ` Michael D Kinney
2023-05-24 21:55       ` [edk2-devel] " Pedro Falcato
2023-05-25  0:23         ` Michael D Kinney
2023-05-25  9:44           ` Pedro Falcato
2023-05-25 17:06             ` Michael D Kinney
2023-05-25 17:43               ` Oliver Smith-Denny
2023-05-25 18:01                 ` Pedro Falcato [this message]
2023-05-25 18:22                   ` Michael D Kinney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKbZUD2q95W7WEdiYqiXSoOt24aAvsOgn+0w34zuMtavMxi6cA@mail.gmail.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox