Skip to content

Refactor CMake code related to compiler warnings, enable some additional warnings#6355

Merged
SiarheiFedartsou merged 40 commits intomasterfrom
sf-warnings
Sep 30, 2022
Merged

Refactor CMake code related to compiler warnings, enable some additional warnings#6355
SiarheiFedartsou merged 40 commits intomasterfrom
sf-warnings

Conversation

@SiarheiFedartsou
Copy link
Copy Markdown
Member

@SiarheiFedartsou SiarheiFedartsou commented Sep 1, 2022

Issue

Doesn't fix anything really important, but allows to better control warnings in the future.

Tasklist

Requirements / Relations

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

include (CheckCCompilerFlag)

# Try to add -Wflag if compiler supports it
macro (add_warning flag)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

protected:
NodeIterator number_of_nodes;
EdgeIterator number_of_edges;
NodeIterator number_of_nodes = 0;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We had maybe-uninitialized disabled, not sure if there was really problem here, but looks suspicious.

@SiarheiFedartsou SiarheiFedartsou marked this pull request as ready for review September 9, 2022 14:28
// This is needed because libstdc++ itself uses this API - its not
// just an issue of your code using it, ughhh

#pragma GCC diagnostic push
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can probably remove this whole file.
We're not supporting GCC versions this old anymore.

add_warning(logical-op)
add_warning(maybe-uninitialized)
add_warning(misleading-indentation)
add_warning(no-return-local-addr)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be a no_warning ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is confusing :) no- prefix is part of warning name, i.e. it doesn't mean we are disabling it, I'll add a comment

@SiarheiFedartsou SiarheiFedartsou merged commit 902bfc7 into master Sep 30, 2022
mattwigway pushed a commit to mattwigway/osrm-backend that referenced this pull request Jul 20, 2023
@DennisOSRM DennisOSRM deleted the sf-warnings branch May 3, 2024 12:29
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