Skip to content

Adds cmake option ENABLE_COMPILER_WARNINGS and fixes compiler warnings#2035

Merged
purew merged 2 commits intovalhalla:masterfrom
purew:compile-with-warnings
Nov 15, 2019
Merged

Adds cmake option ENABLE_COMPILER_WARNINGS and fixes compiler warnings#2035
purew merged 2 commits intovalhalla:masterfrom
purew:compile-with-warnings

Conversation

@purew
Copy link
Copy Markdown
Contributor

@purew purew commented Nov 7, 2019

Enables this option on build-debug in CI which is deemed the one
build to decide on what constitutes a warning.

Fixes #2027

Tasklist

  • Add tests
  • Review - you must request approval to merge any PR to master
  • Add #fixes with the issue number that this PR addresses
  • Generally use squash merge to rebase and clean comments before merging
  • Update the changelog
  • Update relevant documentation

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@purew purew requested review from danpaz and kevinkreiser November 7, 2019 22:59
danpaz
danpaz reviewed Nov 7, 2019
View reviewed changes
Comment thread valhalla/sif/edgelabel.h Outdated
@purew purew requested review from dgearhart and gknisely November 7, 2019 23:24
@purew purew force-pushed the compile-with-warnings branch from ef6d731 to f1994bf Compare November 8, 2019 21:58
@danpaz
Copy link
Copy Markdown
Collaborator

danpaz commented Nov 11, 2019

Running on MacOS with clang I get a few errors we don't see in gcc. I understand we won't track compiler warnings in CI for Mac so would it make sense to fix these here or in a separate PR?

In file included from /Users/danielpaz-soldan/code/valhalla/src/worker.cc:7:
In file included from /Users/danielpaz-soldan/code/valhalla/valhalla/baldr/location.h:8:
/Users/danielpaz-soldan/code/valhalla/valhalla/midgard/pointll.h:89:9: error: 'valhalla::midgard::PointLL::Distance' hides overloaded virtual function [-Werror,-Woverloaded-virt$
al]
  float Distance(const PointLL& ll2) const;
        ^
/Users/danielpaz-soldan/code/valhalla/valhalla/midgard/point2.h:100:17: note: hidden overloaded virtual function 'valhalla::midgard::Point2::Distance' declared here: type mismat$
h at 1st parameter ('const valhalla::midgard::Point2 &' vs 'const valhalla::midgard::PointLL &')
  virtual float Distance(const Point2& p) const;
                ^
In file included from /Users/danielpaz-soldan/code/valhalla/src/worker.cc:7:
In file included from /Users/danielpaz-soldan/code/valhalla/valhalla/baldr/location.h:8:
/Users/danielpaz-soldan/code/valhalla/valhalla/midgard/pointll.h:97:9: error: 'valhalla::midgard::PointLL::DistanceSquared' hides overloaded virtual function [-Werror,-Woverload$
d-virtual]
  float DistanceSquared(const PointLL& ll2) const;
        ^
/Users/danielpaz-soldan/code/valhalla/valhalla/midgard/point2.h:93:17: note: hidden overloaded virtual function 'valhalla::midgard::Point2::DistanceSquared' declared here: type $
ismatch at 1st parameter ('const valhalla::midgard::Point2 &' vs 'const valhalla::midgard::PointLL &')
  virtual float DistanceSquared(const Point2& p) const;
                ^
In file included from /Users/danielpaz-soldan/code/valhalla/src/worker.cc:7:
In file included from /Users/danielpaz-soldan/code/valhalla/valhalla/baldr/location.h:8:
/Users/danielpaz-soldan/code/valhalla/valhalla/midgard/pointll.h:196:17: error: 'valhalla::midgard::PointLL::IsLeft' hides overloaded virtual function [-Werror,-Woverloaded-virt$
al]
  virtual float IsLeft(const PointLL& p1, const PointLL& p2) const {
                ^
/Users/danielpaz-soldan/code/valhalla/valhalla/midgard/point2.h:168:17: note: hidden overloaded virtual function 'valhalla::midgard::Point2::IsLeft' declared here: type mismatch
at 1st parameter ('const valhalla::midgard::Point2 &' vs 'const valhalla::midgard::PointLL &')
  virtual float IsLeft(const Point2& p1, const Point2& p2) const {
                ^
In file included from /Users/danielpaz-soldan/code/valhalla/src/worker.cc:12:
In file included from /Users/danielpaz-soldan/code/valhalla/valhalla/sif/costfactory.h:9:
In file included from /Users/danielpaz-soldan/code/valhalla/valhalla/sif/autocost.h:9:
In file included from /Users/danielpaz-soldan/code/valhalla/valhalla/sif/dynamiccost.h:10:
In file included from /Users/danielpaz-soldan/code/valhalla/valhalla/baldr/graphtile.h:8:
/Users/danielpaz-soldan/code/valhalla/valhalla/baldr/curler.h:95:32: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
      : owner_(owner), curler_(std::move(owner.acquire())) {
                               ^
/Users/danielpaz-soldan/code/valhalla/valhalla/baldr/curler.h:95:32: note: remove std::move call here
      : owner_(owner), curler_(std::move(owner.acquire())) {
                               ^~~~~~~~~~               ~

I also get errors from rapidjson, a fix for that is in #1864.

@purew
Copy link
Copy Markdown
Contributor Author

purew commented Nov 11, 2019

would it make sense to fix these here or in a separate PR?

No strong opinions either way but smaller PRs that can be merged faster is usually nicer.

kevinkreiser
kevinkreiser reviewed Nov 13, 2019
View reviewed changes
Comment thread CMakeLists.txt Outdated
Copy link
Copy Markdown
Member

@kevinkreiser kevinkreiser Nov 13, 2019

Choose a reason for hiding this comment

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

why not default on? this would be a good way to annoy everyone into keeping the output warning free

Copy link
Copy Markdown
Contributor Author

@purew purew Nov 13, 2019

Choose a reason for hiding this comment

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

We discussed before starting this work that having it default on may be untenable due to the multitudes of platform and toolchains we aim to compile on.

A way out of that is to deem one toolchain the one and require no warnings there while being more lenient elsewhere.

kevinkreiser
kevinkreiser reviewed Nov 13, 2019
View reviewed changes
Comment thread src/thor/triplegbuilder.cc Outdated
kevinkreiser
kevinkreiser reviewed Nov 13, 2019
View reviewed changes
Comment thread src/worker.cc Outdated
kevinkreiser
kevinkreiser previously approved these changes Nov 13, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@kevinkreiser kevinkreiser left a comment

Choose a reason for hiding this comment

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

other than nitpicks looks ok to me

@purew purew force-pushed the compile-with-warnings branch 2 times, most recently from 115f072 to 7e32818 Compare November 13, 2019 21:57
purew added 2 commits November 15, 2019 10:34
@purew
Adds cmake option ENABLE_COMPILER_WARNINGS and fixes compiler warnings
819326b 
Enables this option on `build-debug` in CI which is deemed _the one_
build to decide on what constitutes a warning.

Fixes valhalla#2027
@purew
Nits: Improves error messages
1d76bb7
@purew purew force-pushed the compile-with-warnings branch from 7e32818 to 1d76bb7 Compare November 15, 2019 16:11
@purew purew merged commit 9039ae0 into valhalla:master Nov 15, 2019
@purew purew deleted the compile-with-warnings branch November 15, 2019 21:40
@danpaz danpaz mentioned this pull request Oct 12, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dnesbitt61 dnesbitt61 dnesbitt61 left review comments

@danpaz danpaz danpaz approved these changes

@kevinkreiser kevinkreiser kevinkreiser left review comments

@dgearhart dgearhart Awaiting requested review from dgearhart

@gknisely gknisely Awaiting requested review from gknisely

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enable compiler warnings

4 participants

@purew @danpaz @kevinkreiser @dnesbitt61