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

Fixed GCC version test. #771

Closed
wants to merge 6 commits into from
Closed

Fixed GCC version test. #771

wants to merge 6 commits into from

Conversation

Remotion
Copy link
Contributor

@Remotion Remotion commented Jun 8, 2018

Please see here for more info #770

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. One comment and please rebase since quotation functionality has already been merged.

@@ -51,7 +51,7 @@
# define FMT_ICC_VERSION 0
#endif

#if (defined(__GNUC__) && !defined(__clang__) && __GNUC__ >= 406) || \
#if (defined(__GNUC__) && !defined(__clang__) && (__GNUC__ * 100 + __GNUC_MINOR__) >= 406) || \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could use FMT_GCC_VERSION instead of __GNUC__ * 100 + __GNUC_MINOR__ (possibly after reordering defines).

Copy link
Contributor

@eliaskosunen eliaskosunen Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FMT_GCC_VERSION is defined in core.h, which is included after this. This also needs to be before the inclusion of core.h to disable warnings from that header. On your first review in #736 you said that FMT_GCC_VERSION needs to be in core.h.

So that leaves two options that I can think of:

  1. Leave it as is
  2. Duplicate these warning-disabling maneuvers in both core.h and format.h and move the inclusion of core.h before the disabling of the warnings in format.h, so both can use FMT_GCC_VERSION, like this:
// core.h
#define FMT_GCC_VERSION /* ... */

#if FMT_GCC_VERSION
// disable
#endif

// ...

#if FMT_GCC_VERSION
// re-enable
#endif
// format.h

#include "core.h"

#if FMT_GCC_VERSION
// disable
#endif

// ...

#if FMT_GCC_VERSION
// re-enable
#endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I said that very incoherently. What I was trying to say was that the warnings need to be disabled before the inclusion of core.h, but FMT_GCC_VERSION is defined in core.h. So we can either not use FMT_GCC_VERSION in disabling the warnings or disable them in both headers. The latter would probably be preferable, because now the warnings aren't disabled at all if you only include core.h.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can live without disabling these warnings in fmt/core.h for now, so I suggest just moving fmt/core.h to the front and using FMT_GCC_VERSION here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original point of this PR was to fix the version check so there wouldn't be any -Wshadow warnings.

Copy link
Contributor

@vitaut vitaut Jun 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and this will fix the reported warnings in fmt/format.h (but not in fmt/core.h which is OK since no such warnings were reported).

@Remotion Remotion mentioned this pull request Jun 9, 2018
@Remotion Remotion closed this Jun 9, 2018
@eliaskosunen eliaskosunen mentioned this pull request Jun 11, 2018
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