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 an another -Wundef #730

Merged
merged 1 commit into from
May 9, 2018
Merged

Fix an another -Wundef #730

merged 1 commit into from
May 9, 2018

Conversation

eliaskosunen
Copy link
Contributor

Here's yet another -Wundef PR. Today we have FMT_CLANG_VERSION being used without being defined if FMT_GCC_VERSION is less than 600.

Even though the usage of an undefined macro has the well-defined semantics of evaluating to zero, I think doing so is bad practice and ignoring those warnings can easily lead to unintended behavior. I think that a project as widely used as fmt should compile cleanly with the most aggressive warning levels. To accomplish this, I really think CI should be more strict with warnings and use more compilers than just g++-6. I can work on a PR on that as well if you'd like.

@eliaskosunen
Copy link
Contributor Author

eliaskosunen commented May 7, 2018

For example, currently when compiling with clang you'll receive a boatload of warnings, including -Wsign-conversion, because these are only disabled for g++.

I've had to resort to doing this before including fmt:

#if defined(__GNUC__) || defined(__clang__)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wswitch-enum"
#pragma GCC diagnostic ignored "-Wsign-conversion"
#pragma GCC diagnostic ignored "-Wconversion"
#pragma GCC diagnostic ignored "-Wformat-nonliteral"
#pragma GCC diagnostic ignored "-Wmissing-noreturn"
#pragma GCC diagnostic ignored "-Wunused-parameter"
#pragma GCC diagnostic ignored "-Wdeprecated"
#pragma GCC diagnostic ignored "-Wshadow"
#pragma GCC diagnostic ignored "-Wundef"

#if (defined(__GNUC__) && !defined(__clang__)) || \
    (defined(__clang__) && __clang_major__ > 4)
#pragma GCC diagnostic ignored "-Wzero-as-null-pointer-constant"
#endif
#if defined(__clang__) && __clang_major__ >= 4
#pragma GCC diagnostic ignored "-Wdeprecated-dynamic-exception-spec"
#endif

#ifdef __clang__
#pragma GCC diagnostic ignored "-Wshadow-field-in-constructor"
#pragma GCC diagnostic ignored "-Wdocumentation-unknown-command"
#pragma GCC diagnostic ignored "-Wunused-member-function"
#pragma GCC diagnostic ignored "-Wshorten-64-to-32"
#pragma GCC diagnostic ignored "-Wextra-semi"
#else
#pragma GCC diagnostic ignored "-Wctor-dtor-privacy"
#endif
#endif  // defined(__GNUC__) || defined(__clang__)

I think these should be either fixed or explicitly disabled in the header.

@vitaut
Copy link
Contributor

vitaut commented May 9, 2018

I really think CI should be more strict with warnings and use more compilers than just g++-6. I can work on a PR on that as well if you'd like.

I think it would be useful to add an old gcc compiler (4.4.x) to the CI config. PRs for this and stricter warnings are welcome.

@vitaut vitaut merged commit a4e4f74 into fmtlib:master May 9, 2018
@vitaut
Copy link
Contributor

vitaut commented May 9, 2018

Merged, thanks!

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