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 warnings on clang 3.9 #1529

Closed
wants to merge 1 commit into from

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Oct 12, 2016

I guess we extensively use constructs like this:

#if SOMETIMES_DEFINED > 3

as shorthand for

#if defined(SOMETIMES_DEFINED) && SOMETIMES_DEFINED > 3

I was never 100% sure that the former was legal, but the compilers didn't
complain and I preferred it over the more verbose equivalent.

But apparently, it's just lax compilers, and in the latest Clang 3.9,
doing this (assuming "expansion to defined") is a warning, and of course
we usually treat warnings as errors.

It's going to be a lot of work to track all of these down and fix them,
especially before clang 3.9 has a Homebrew package (without it, I'm
operating blind, both on my laptop and on the TravisCI builds). So for
now, we'll settle for just disabling that warning, and slowly fix up the
guards later.

I guess we extensively use constructs like this:

    #if SOMETIMES_DEFINED > 3

as shorthand for

    #if defined(SOMETIMES_DEFINED) && SOMETIMES_DEFINED > 3

I was never 100% sure that the former was legal, but the compilers didn't
complain and I preferred it over the more verbose equivalent.

But apparently, it's just lax compilers, and in the latest Clang 3.9,
doing this (assuming "expansion to defined") is a warning, and of course
we usually treat warnings as errors.

It's going to be a lot of work to track all of these down and fix them,
especially before clang 3.9 has a Homebrew package (without it, I'm
operating blind, both on my laptop and on the TravisCI builds). So for
now, we'll settle for just disabling that warning, and slowly fix up the
@lgritz
Copy link
Collaborator Author

lgritz commented Oct 12, 2016

Ashley Retallack says this fixes the problem (which he reported).
I'll merge as soon as the CI tests all come back.

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 12, 2016

Merged.

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.

1 participant