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

Re-enable module testing #2332

Merged
merged 1 commit into from
Jun 1, 2021
Merged

Conversation

DanielaE
Copy link
Contributor

Prepare for compilation with gcc (modules branch).

@DanielaE
Copy link
Contributor Author

This depends on #2324

@DanielaE DanielaE marked this pull request as ready for review May 31, 2021 16:16
@DanielaE
Copy link
Contributor Author

DanielaE commented May 31, 2021

I've force-pushed my local work-in-progress branch with a lot of additional tests to see if module testing works. It does: https://github.com/DanielaE/fmt/runs/2712421080

@vitaut
Copy link
Contributor

vitaut commented Jun 1, 2021

CI is still failing.

@DanielaE DanielaE force-pushed the feature/enable-module-tests branch from f5965b3 to b6b8e68 Compare June 1, 2021 16:33
@DanielaE
Copy link
Contributor Author

DanielaE commented Jun 1, 2021

Interesting. Never seen this. Is this a GTest feature to compare C-strings by identity rather than by equality?

@@ -405,9 +411,9 @@ TEST(module_test, color) {

TEST(module_test, cstring_view) {
fmt::cstring_view nsv("fmt");
EXPECT_EQ("fmt", nsv.c_str());
EXPECT_EQ("fmt", std::string_view(nsv.c_str()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using EXPECT_STREQ instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually no, this checks the pointer comparison so the correct fix is to move "fmt" into a separate variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My new code cannot fall into the identity trap anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean by identity trap but this must compare pointers, not strings, something like:

auto s = "fmt";
EXPECT_EQ(s, fmt::cstring_view(s).c_str());

@DanielaE DanielaE force-pushed the feature/enable-module-tests branch from b6b8e68 to cc7b64f Compare June 1, 2021 17:27
Prepare for compilation with gcc (modules branch).
@DanielaE DanielaE force-pushed the feature/enable-module-tests branch from cc7b64f to 4f7ea16 Compare June 1, 2021 18:00
@vitaut vitaut merged commit 70e67ae into fmtlib:master Jun 1, 2021
@vitaut
Copy link
Contributor

vitaut commented Jun 1, 2021

Thank you!

@DanielaE DanielaE deleted the feature/enable-module-tests branch June 2, 2021 04:59
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.

2 participants