Fix DLL visibility of explicit instantiation "declaration" of internal::basic_data<void> in header format.h and the explicit instantiation "definition" in format.cc

[denchat]

In format.h, the explicit instantiation declaration of internal::basic_data should mirror DLL visibility of FMT_API

As the explicit instantiation declaration of internal::basic_data<void> in format.h, this explicit instantiation definition should mirror FMT_API also.

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

[vitaut]

Thanks for the PR, but I don't think FMT_API should be repeated there (see inline comment). What problem are you trying to solve?

[vitaut]

I don't think this is right, e.g. https://godbolt.org/z/kGqqeS gives

<source>(6): warning C4910: 'basic_data<void>': '__declspec(dllexport)' and 'extern' are incompatible on an explicit instantiation

[denchat]

Thank you. You are right. It seems the instantiation declaration implicitly has got the same __declspec(dllexport) from the class template definition.

So we can resolve this at the class template definition alone.

It looks like travis-ci compilation failed by some reason. How can I make them recompile?

[denchat]

My problem is when I tried to compile this code on Windows

#include <cstdio>
#include <fmt/printf.h>

int main()
{
  fmt::printf("Hello, %s!\n", "world");
}

The linker can not find internal::basic_data<void>::ZERO_OR_POWERS_OF_10_32

lld-link: error: undefined symbol: __declspec(dllimport) public: static unsigned int const *const fmt::v5::internal::basic_data<void>::ZERO_OR_POWERS_OF_10_32

This is because we were trying to dllexport : explicit class template instantiation "declaration"

template <typename T = void> struct __declspec(dllexport) basic_data {
  static const uint64_t POWERS_OF_10_64[];
  static const uint32_t ZERO_OR_POWERS_OF_10_32[];
  static const uint64_t ZERO_OR_POWERS_OF_10_64[];
  static const uint64_t POW10_SIGNIFICANDS[];
  static const int16_t POW10_EXPONENTS[];
  static const char DIGITS[];
  static const char HEX_DIGITS[];
  static const char FOREGROUND_COLOR[];
  static const char BACKGROUND_COLOR[];
  static const char RESET_COLOR[5];
  static const wchar_t WRESET_COLOR[5];
};

#if FMT_USE_EXTERN_TEMPLATES
extern template struct basic_data<void>;
#endif

but not the explicit class template instantiation "definition" in format.cc

template struct internal::basic_data<void>;

---

When doing DLL on extern template instantiation, we have to do something like this.

#ifdef XXXX_BUILD
    #define XXXX_EXPORT __declspec(dllexport)
    #define XXXX_EXTERN
#else
    #define XXXX_EXPORT __declspec(dllimport)
    #define XXXX_EXTERN extern
#endif
....
XXXX_EXTERN template class XXXX_EXPORT YourClass<double>;

REF: https://stackoverflow.com/a/46392757/6370128

So we want something like this

#define FMT_API __declspec(dllexport)

template <typename T = void> struct basic_data {};

extern template struct basic_data<void>;

//
//  ...far far away in the same translation unit (in case of format.cc)
//  (or expecting code below in another translation unit (in case of posix.cc) )
//

template struct FMT_API basic_data<void>;

godbolt

[vitaut]

I don't think extern is actually useful here, so I suggest just removing it together with the whole FMT_USE_EXTERN_TEMPLATES block.

[denchat]

extern is telling other translation units that they all could rely on the definition of internal::basic_data<void> in format.cc. So that all translation units using internet::basic_dat<void>, except format.cc, don't need to instantiate it by themselves. This makes compilation faster. This is the purpose of c++11 explicit class template declaration feature.

If extern in format.h is removed, so it becomes the definition itself. All translation units using format.h will be required to instantiate internal::basic_data<vod> every single time which that doesn't need to, only if using c++11 explicit class template declaration feature.

Besides, what about the definition in format.cc? Now there would be already another definition in format.h, so the definition in format.cc must be removed altogether with extern then.

However, if you are affirmed that the explicit class template declaration feature would make lesser benefits than the complexity it'd gain here, I will make the definition stay in format.h as you suggested.

[vitaut]

You are right, it's better to keep extern template, but please remove the macro FMT_USE_EXTERN_TEMPLATES. We already require C++11 and this feature is widely available. Hopefully this will simplify the logic around FMT_API a bit.

[denchat]

I've already removed the whole FMT_USE_EXTERN_TEMPLATES block.

Anyway, I guess

• __declspec(DLL) api of the template definition having any extern declarations must have its own condition other than FMT_API.
• the extern in front of extern template declaration itself must also have its own condition as well.

I'm not any good at naming. I only could come up with FMT_EXTERN_TEMPLATE_API and FMT_EXTERN respectively and make them added in core.h. Any suggestions are welcome.

[vitaut]

Merged, thanks a lot!

[NikitaFeodonit]

It should be like this:

#if !defined(FMT_HEADER_ONLY) && defined(_WIN32)
#  ifdef FMT_EXPORT
#    define FMT_API __declspec(dllexport)
#    define FMT_EXTERN_TEMPLATE_API FMT_API
#  elif defined(FMT_SHARED)
#    define FMT_API __declspec(dllimport)
#    define FMT_EXTERN_TEMPLATE_API FMT_API
#  endif
#endif

With the addition

#    define FMT_EXTERN_TEMPLATE_API FMT_API

the MinGW shared build is fixed.

[vitaut]

@NikitaFeodonit, if you get an error in MinGW please open a new issue (or a pull request) with more details.