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

Enable error when GCC/clang print warning #4843

Merged
merged 19 commits into from
Feb 9, 2018
Merged

Conversation

TheMarex
Copy link
Member

@TheMarex TheMarex commented Feb 1, 2018

Issue

As replacement for this pr we are going to be a lot stricter about the warnings we do enable and will make the build fail so that we can catch it in CI.

Tasklist

  • review
  • adjust for comments
  • cherry pick to release branch

Requirements / Relations

Replacement for #4495

@mloskot
Copy link
Member

mloskot commented Feb 1, 2018

@TheMarex Shall I close the old one of mine?

@TheMarex
Copy link
Member Author

TheMarex commented Feb 5, 2018

This PR is blocked by #4849 since we require a higher compiler version to trim down the false positives.

@springmeyer
Copy link
Contributor

Nice work getting this going!

@TheMarex TheMarex force-pushed the build/error_on_warn branch from c95d4d8 to 0248b52 Compare February 8, 2018 20:23
@mloskot
Copy link
Member

mloskot commented Feb 9, 2018

I could help cleaning/fixing warnings reported by MSVC.
Shall I wait until this PR has been merged (some warnings may overlap on multiple platforms, of course) ?

@TheMarex TheMarex force-pushed the build/error_on_warn branch from 7146357 to 295d615 Compare February 9, 2018 10:06
@TheMarex
Copy link
Member Author

TheMarex commented Feb 9, 2018

@mloskot yes a MSVC PR would be great!

@mloskot
Copy link
Member

mloskot commented Feb 9, 2018

@TheMarex

@mloskot yes a MSVC PR would be great!

Good. I assume I should wait until this is merged, then I can clean any remaining MSVC annoyances.

Copy link
Contributor

@oxidase oxidase left a comment

Choose a reason for hiding this comment

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

Looks good to me! Really nice to have zero warnings policy

.travis.yml Outdated
@@ -350,7 +350,7 @@ install:
- mkdir example/build && pushd example/build
- export PKG_CONFIG_PATH=${OSRM_INSTALL_DIR}/lib/pkgconfig
- cmake .. -DCMAKE_BUILD_TYPE=${BUILD_TYPE}
- make --jobs=${JOBS}
- make --jobs=${JOBS} VERBOSE=1
Copy link
Contributor

@oxidase oxidase Feb 9, 2018

Choose a reason for hiding this comment

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

is VERBOSE=1 needed on master or it is just a left over?

Copy link
Member Author

Choose a reason for hiding this comment

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

Leftover, good catch forgot about this commit.

@TheMarex TheMarex force-pushed the build/error_on_warn branch from 295d615 to df1b06b Compare February 9, 2018 15:37
@TheMarex TheMarex merged commit 1aed135 into master Feb 9, 2018
@TheMarex TheMarex deleted the build/error_on_warn branch February 9, 2018 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants