8000
Skip to content

Re-enable module testing#2332

Merged
vitaut merged 1 commit intofmtlib:masterfrom
DanielaE:feature/enable-module-tests
Jun 1, 2021
Merged

Re-enable module testing#2332
vitaut merged 1 commit intofmtlib:masterfrom
DanielaE:feature/enable-module-tests

Conversation

@DanielaE
Copy link
Copy Markdown
Contributor

Prepare for compilation with gcc (modules branch).

@DanielaE
Copy link
Copy Markdown
Contributor Author

This depends on #2324

@DanielaE DanielaE marked this pull request as ready for review May 31, 2021 16:16
@DanielaE
Copy link
Copy Markdown
Contributor Author
DanielaE commented May 31, 2021 < 8000 span class="js-comment-edit-history">

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
Copy Markdown
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
Copy Markdown
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?

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

0