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 more warnings/make them errors per mapbox/cpp#37 #14

Merged
merged 7 commits into from
Oct 17, 2017
Merged

Conversation

springmeyer
Copy link
Contributor

Adding more default warnings (and making warnings errors) per mapbox/cpp#37

@springmeyer springmeyer added this to the v1.0.0 milestone Oct 16, 2017
@springmeyer
Copy link
Contributor Author

All done on the "enabling more warnings" side here. Now over to you @artemp, to decide how to fix the new warnings that have come up.

@artemp
Copy link
Contributor

artemp commented Oct 16, 2017

@springmeyer - all warnings fixed but g++ doesn't understand some compile flags, so over back to you :)

@springmeyer
Copy link
Contributor Author

@artemp thank you - curious, why are some things throwing with throw std::runtime_error("Not implemented!"); (and therefore previously warning on unused args). Is this by design?

@artemp
Copy link
Contributor

artemp commented Oct 16, 2017

@springmeyer - yes, those structures are helpers allowing incomplete implementations (not all possible permutations of geometry types). This is intentional design feature to keep binary size under control. I'll take a look again to see if this mechanism can be implemented through meta-programming (compile time).

@springmeyer
Copy link
Contributor Author

Okay, good to know @artemp - this would be great to document. How about you add a mention to the readme about this in design decision section?

@springmeyer
Copy link
Contributor Author

@artemp okay, I pushed a fix to this branch so that we no longer apply -Wmost to g++ builds. Back to you again: the g++ builds are not emitting warnings on the code inside include/boost. Seems like we have two options to solve this. We could ignore the warnings from the vendered boost headers by doing:

- include_directories("${PROJECT_SOURCE_DIR}/include")
+ include_directories(SYSTEM "${PROJECT_SOURCE_DIR}/include/boost")
+ include_directories("${PROJECT_SOURCE_DIR}/include/mapbox")

Or we could fix the warnings. I noticed you've already modified the files inside include/boost. Which makes me lean towards treating these files as owned by this repo and therefore we should consider fixing the warnings?

@artemp
Copy link
Contributor

artemp commented Oct 16, 2017

@springmeyer - on my TODO list

@artemp artemp merged commit 5dfca06 into master Oct 17, 2017
@artemp artemp deleted the warnings branch October 17, 2017 10:43
@springmeyer
Copy link
Contributor Author

Changes look great - thanks @artemp 🙌

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