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

Include test sources to pick up functions and classes from the module… #2356

Merged
merged 1 commit into from
Jul 3, 2021

Conversation

DanielaE
Copy link
Contributor

… rather than from the non-modular library that's baked into the test-main library.

This avoids linker problems:

  • strong ownership model: missing linker symbols
  • weak ownership model: duplicate linker symbols

@DanielaE
Copy link
Contributor Author

DanielaE commented Jun 14, 2021

I observed this linker problem for the first time when experimenting with this consteval thing in msvc.

Comment on lines 38 to 53
#ifndef FMT_POSIX
# if defined(_WIN32) && !defined(__MINGW32__)
# define FMT_POSIX(call) _##call
# else
# define FMT_POSIX(call) call
# endif
#endif
#ifndef _WIN32
# define FMT_RETRY_VAL(result, expression, error_result) \
do { \
(result) = (expression); \
} while ((result) == (error_result) && errno == EINTR)
#else
# define FMT_RETRY_VAL(result, expression, error_result) result = (expression)
#endif
#define FMT_RETRY(result, expression) FMT_RETRY_VAL(result, expression, -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this copy-pasted here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These 2 macros are defined in os.h which cannot be included because it would reintroduce parts of {fmt} into the TU with different linkage and/or mangling. Avoiding such problems is the whole point of the PR. Unfortunately, pieces of the library are used in the implementation of the test support facilities of the library itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copying FMT_POSIX is OK but I suggest inlining the retry loop (without the conditional compilation since it should work on Windows too) into the only place where it is used:

FMT_RETRY(result, fflush(file_));

Also please remove this while at it:

fmt/test/gtest-extra.cc

Lines 35 to 37 in 889bbf2

# if EOF != -1
# error "FMT_RETRY assumes return value of -1 indicating failure"
# endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@DanielaE DanielaE force-pushed the fix/linker-symbols-in-module-test branch from 32c40db to 42cd1c4 Compare July 3, 2021 08:36
@DanielaE
Copy link
Contributor Author

DanielaE commented Jul 3, 2021

The changes are incorporated, the PR is rebased onto HEAD

FMT_RETRY(result, fflush(file_));
do {
result = fflush(file_);
} while (result == -1 && errno == EINTR);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use EOF instead of -1 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

… rather than from the non-modular library which is baked into the `test-main` library.

This averts linker problems:
 - strong ownership model: missing linker symbols
 - weak ownership model: duplicate linker symbols

Simplify `gtest-extra.cc` while at it.
@DanielaE DanielaE force-pushed the fix/linker-symbols-in-module-test branch from 42cd1c4 to 1737f32 Compare July 3, 2021 13:57
@vitaut vitaut merged commit f2b03fa into fmtlib:master Jul 3, 2021
@vitaut
Copy link
Contributor

vitaut commented Jul 3, 2021

Thank you!

@DanielaE DanielaE deleted the fix/linker-symbols-in-module-test branch July 3, 2021 15:31
PoetaKodu pushed a commit to pacc-repo/fmt that referenced this pull request Nov 11, 2021
… rather than from the non-modular library which is baked into the `test-main` library. (fmtlib#2356)

This averts linker problems:
 - strong ownership model: missing linker symbols
 - weak ownership model: duplicate linker symbols

Simplify `gtest-extra.cc` while at it.
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