Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some functions when using cgreen with its namespace in C++. #325

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TommyJC
Copy link
Contributor

@TommyJC TommyJC commented Jan 7, 2024

Hello,

Using cgreen for C++ without including using namespace cgreen in the code results in compilation errors when one attempts to use cgreen::assert_that().

I just prefixed some things with cgreen:: and added namespace cgreen { where appropriate.

The changes from this patch fixed the parts of cgreen I utilized but I haven't been able to test everything in cgreen.

Ideally I wanted to add the cgreen namespace to TestSuite in include/cgreen/internal/suite_internal.h as well but since that has namespace cgreen { later, I take it there was a good reason not to add TestSuite to the cgreen namespace.

Those who are applying using namespace cgreen to get around the compilation error should see no difference.

@thoni56
Copy link
Contributor

thoni56 commented Feb 7, 2025

Sorry, for the belated reply here. Cgreen has been off my radar for a while.

Thanks for this, I think this looks good, but as I'm not fully fluent in all the ins and outs of C++ that's more of an amatuers opinion.

Ideally I wanted to add the cgreen namespace to TestSuite in include/cgreen/internal/suite_internal.h as well but since that has namespace cgreen { later, I take it there was a good reason not to add TestSuite to the cgreen namespace.

You shouldn't be so confident in earlier decisions ;-) Have you tried making that change?

Perhaps @matthargett has some input on that "decision"?

@matthargett
Copy link
Contributor

There were two main rules, but @strtok or @lastcraft might remember more:

  1. No using statements in header files. Do not pollute implicit namespace inclusion by someone using the framework.

  2. Avoid duplicating code/declarations where possible. Want want to service both languages with minimal maintenance and overhead for us and flat C users.

I think this PR works, but maybe we could use a __CGREEN_NAMESPACE_PREFIX macro that's defined to empty for non-C++ so that we don't need to duplicate all the definitions for each language? I realize we may not have done that consistently when we first did the work 16 years ago, but that's my only minor suggestion with older (never wiser ;) ) eyes.

If there's any contention about using a macro to help prefix the namespace, feel free to merge as-is.

@TommyJC
Copy link
Contributor Author

TommyJC commented Feb 10, 2025

Hi,

Just to clarify there are no "using" statements in the patch and I concur that "No using statements in header files." is a good rule to live by.

What I meant by the statement,

Those who are applying using namespace cgreen to get around the compilation error should see no difference.

is that anyone who did use using namespace cgreen in their own code to get around the issue the patch was meant to fix shouldn't see a difference.


As for the macro solution, I agree that that is a much cleaner and more elegant solution since it wouldn't require copying and pasting like I did with mine.

Where would that macro best be defined?

Edit: Unquoted "is that anyone who did ..." bit sentence.

@thoni56
Copy link
Contributor

thoni56 commented Feb 15, 2025

Using a macro to make the definitions only occur once seems cool and practical at first. But thinking a bit more, I realized that having a simple #define will not work because we cannot do

__CGREEN_NAMESPACE_PREFIXget_test_reporter...

since that will become a whole different identifier. And inserting a space

__CGREEN_NAMESPACE_PREFIX get_test_reporter...

will not work as AFAIK you are not allowed to have a space between the :: and the symbol after it.

An option would be to create a more complex macro, something like

#ifdef __cplusplus
#define CGREEN_SYMBOL(x) cgreen::x
#else
#define CGREEN_SYMBOL(x) x
#endif

But it feels this will takes away a lot of the readability of these declarations. So it comes down to, as it often does, readability or terseness/DRY.

I hope someone can either correct me or come up with a better solution because we have the same thing in legacy.h.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants