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 clang warnings #403

Closed
wants to merge 1 commit into from
Closed

fix clang warnings #403

wants to merge 1 commit into from

Conversation

Lectem
Copy link
Contributor

@Lectem Lectem commented Oct 30, 2016

When building with c++14 and -Weverything, I encountered the following warnings which are now fixed :

  • dynamic exception specifications are deprecated [-Wdeprecated]
  • 'FMT_USE_OVERRIDE' is not defined, evaluates to 0 [-Wundef]
  • definition of implicit copy constructor for 'FormatError' is deprecated because it has a user-declared destructor [-Wdeprecated]

@Lectem
Copy link
Contributor Author

Lectem commented Oct 30, 2016

To make GCC compile while removing the deprecated warning of throw() for clang, I had to introduce a new define FMT_DESTRUCTOR_NOEXCEPT as the header would still use throw() even when exceptions are disabled. It has the same behaviour as FMT_NOEXCEPT for compilers other than GCC.
It seems to be more of a libstdc++11 problem though, I'm not sure if it could pose problems for some versions of clang.

@foonathan
Copy link
Contributor

foonathan commented Oct 30, 2016

I am personally not happy with the destructor noexcept workaround (but @vitaut opinion counts).
The problem could be solved more elegantly if we remove the destructor in the exception classes, because the implicitly generated one should be throw() in those cases automatically (IIRC). I'm guessing the destructors are there to avoid the weak vtable warning?

Anyways, I'd merge that PR (and would include it in the 3.0.1 I was just about to release), but I'm waiting on @vitaut's opinion here.

@Lectem
Copy link
Contributor Author

Lectem commented Oct 30, 2016

I wanted to remove the destructors at first too but I suspected they were added for a reason, it would be nice to confirm why.

@vitaut
Copy link
Contributor

vitaut commented Oct 31, 2016

Merged with minor changes in a5b9611. Thanks, @Lectem!

@foonathan, I'd prefer to remove dtors too, but as you correctly guessed they were added to prevent -Wweak-vtables warning in clang (b26e76e). Alternative suggestions are welcome.

@vitaut vitaut closed this Oct 31, 2016
@vitaut
Copy link
Contributor

vitaut commented Oct 31, 2016

Hmm, the fix caused failures on Travis, so I moved it to the https://github.com/fmtlib/fmt/tree/warnings branch for further investigation.

@vitaut
Copy link
Contributor

vitaut commented Nov 1, 2016

Merged to master: 8f455c1.

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.

3 participants