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

New warning with Clang for non-header only mode #318

Closed
mwinterb opened this issue May 7, 2016 · 4 comments
Closed

New warning with Clang for non-header only mode #318

mwinterb opened this issue May 7, 2016 · 4 comments

Comments

@mwinterb
Copy link
Contributor

mwinterb commented May 7, 2016

All usages of "Data" in format.h trigger a new warning in clang, -Wundefined-var-template.

The fix is easy enough, after "typedef BasicData<> Data;", add:

#ifndef FMT_HEADER_ONLY
extern template struct BasicData<void>;
#endif

But I don't have access to enough compilers to identify the necessary feature test for extern template support, so I don't feel comfortable submitting a PR. GCC's C++11 status page just says "Yes" (going back to at least 4.3). Clang reports 2.9. I think MSVC has had support for them for "ages", possibly as far back as VS 2003, before they were standard. Intel is at least 11.1. Curiously, Visual Studio 2015's intellisense compiler complains 'extern template' cannot follow explicit instantiation of class "fmt::internal::BasicData<void>", which would suggest possibly this instead:

#ifndef FMT_HEADER_ONLY
#ifndef __INTELLISENSE__
extern template struct BasicData<void>;
#endif
#endif

Since it's a clang-only warning, one option would be to just use FMT_HAS_FEATURE, but I can't find a relevant feature test listed here:
http://clang.llvm.org/docs/LanguageExtensions.html#checks-for-standard-language-features
Another cheap option would be just be a check for __clang__ being defined, but that would prevent prevent working with a pre-2.9 Clang.

The warning was added to clang here http://reviews.llvm.org/rL266719 and discussed here http://reviews.llvm.org/D12326

Sorry, this is far too many words for a simple thing, I just didn't feel comfortable submitting a PR for something I was pretty certain wouldn't work everywhere, and I don't have enough experience supporting multiple compilers at once to make an optimal choice.

@vitaut
Copy link
Contributor

vitaut commented May 7, 2016

BasicData is not a variable template, so this looks like a bug in clang. What version of clang do you use and what is the full warning message?

@mwinterb
Copy link
Contributor Author

mwinterb commented May 7, 2016

Of course I should have included the full warning message, sorry. The warning is possibly confusingly named, but based on the idea behind the warning seems like it's correctly issued in this case.
One example:

fmt/format.h(839,38): warning : instantiation of variable 'fmt::internal::BasicData<void>::POWERS_OF_10_64' required here, but no definition is available [-Wundefined-var-template]
    return to_unsigned(t) - (n < Data::POWERS_OF_10_64[t]) + 1;
                                       ^
fmt/format.h(826,25):  note: forward declaration of template entity is here
    static const uint64_t POWERS_OF_10_64[];
                          ^
fmt/format.h(839,38):  note: add an explicit instantiation declaration to suppress this warning if 'fmt::internal::BasicData<void>::POWERS_OF_10_64' is explicitly instantiated in another translation unit
    return to_unsigned(t) - (n < Data::POWERS_OF_10_64[t]) + 1;
                                       ^

The clang build is their one of their Windows nightlies based on r268238 from May 2. I'll check with a Debian build (also nightly) hopefully tomorrow. And, yes, it's a nightly, and nightlies can have notable bugs (especially with the extra quirks that are enabled for MSVC compatibility), but a warning saying "this template was implicitly instantiated, but there was no definition found for it in this translation unit" seems correctly issued in this instance.

@vitaut
Copy link
Contributor

vitaut commented May 8, 2016

Thanks for providing additional info. Could you check if 744c282 fixes the problem?

@mwinterb
Copy link
Contributor Author

mwinterb commented May 9, 2016

Yes it did, thank you.

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

No branches or pull requests

2 participants