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 module test odr violations #2414

Merged
merged 1 commit into from
Jul 17, 2021
Merged

Conversation

cdacamar
Copy link
Contributor

@cdacamar cdacamar commented Jul 5, 2021

As of 16.11 Preview 3 the MSVC compiler will no longer be, erroneously, be injecting header include guard macros when importing a module interface. The bugfix exposed some issues related to textually including fmt headers in the test code which causes odr violations with the imported interface.

@DanielaE
Copy link
Contributor

DanielaE commented Jul 14, 2021

@cdacamar
I'm a bit puzzled regarding the proposed changes. I've upgraded from pre-2 (19.29.30130) to pre-3 (19.29.30132) and can confirm that the include guard macros are no longer injected into the importing TU. To codify this improvement, I changed module-test.cc from

#define FMT_OS_H_  // don't pull in os.h directly or indirectly

import fmt;

// check for macros leaking from BMI
static bool macro_leaked =
#if defined(FMT_CORE_H_) || defined(FMT_FORMAT_H_)
    true;
#else
    false;
#endif

to

#define FMT_OS_H_  // don't pull in os.h directly or indirectly

import fmt;

// check for macros leaking from BMI
static bool macro_leaked =
#if defined(FMT_CORE_H_) || defined(FMT_FORMAT_H_) || !defined(FMT_OS_H_)
    true;
#else
    false;
#endif

Beginning with pre-3, include guard macros no longer become #defined nor #undefed by importing a named module.
By defining the include guard for fmt/os.h, no header becomes #included from {fmt} by the code following the import. Every required definition is rather imported from the module, e.g. no more ODR violations. I've checked that explicitly with both dumping the preprocessed code (/P) and applying my fake consteval emulation.

The only other change to compile module-test.cc successfully with pre-3 is in line 80 where the visibility of the non-exported namespace fmt::detail is checked for. The compiler version needs to be bumped up to 192930132.

No changes to other files were necessary.

@cdacamar
Copy link
Contributor Author

@DanielaE while your method works it requires knowing the implementation detail of the header include guards by name (which could change in the future). The proposed solution here relies only on the fact that we understand the contents of fmtlib comes from the module alone so we can forget about the mechanics of #include altogether and simply import the module which provides the library.

@vitaut vitaut merged commit 00235d8 into fmtlib:master Jul 17, 2021
@vitaut
Copy link
Contributor

vitaut commented Jul 17, 2021

Thanks for the fix!

sthagen added a commit to sthagen/fmtlib-fmt that referenced this pull request Jul 17, 2021
fix module test odr violations (fmtlib#2414)
PoetaKodu pushed a commit to pacc-repo/fmt that referenced this pull request Nov 11, 2021
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.

None yet

3 participants