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 -Weffc++ for gcc and clang #4059

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Enable -Weffc++ for gcc and clang #4059

merged 1 commit into from
Feb 20, 2024

Conversation

rouault
Copy link
Member

@rouault rouault commented Feb 20, 2024

No description provided.

Copy link
Contributor

@jjimenezshaw jjimenezshaw left a comment

Choose a reason for hiding this comment

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

I am curious how you ended up here, when there is not code change in the PR (so I assume that the current code works perfectly with that option).

In https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html I read "When selecting this option, be aware that the standard library headers do not obey all of these guidelines; use ‘grep -v’ to filter out those warnings." Are we affected?

@rouault
Copy link
Member Author

rouault commented Feb 20, 2024

I am curious how you ended up here, when there is not code change in the PR (so I assume that the current code works perfectly with that option).

Well, someone makes me resurrect, in a custom branch, autotools support for PROJ master, and autotools builds had -Weffc++ enabled, hence I noticed a few warnings, which I directly fixed in master per 26fb427 and 81fecbc . So this is to avoid such occurrences to happen again

In https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html I read "When selecting this option, be aware that the standard library headers do not obey all of these guidelines; use ‘grep -v’ to filter out those warnings." Are we affected?

I've not noticed that. At worse we can disable it if it becomes an annoyance but this is enabled in GDAL which draws much more stuff than PROJ, so I won't anticipate any issues for PROJ

@rouault rouault merged commit 498d409 into OSGeo:master Feb 20, 2024
23 checks passed
@mwtoews
Copy link
Member

mwtoews commented Feb 21, 2024

It seems that I forgot to check over these flags from https://github.com/OSGeo/PROJ/blob/8.2/configure.ac

Are there others? Here are some that were mentioned in the old configure.ac but not transferred into CMakeLists.txt:

  • -Winit-self
  • -Wshorten-64-to-32
  • -Wlogical-op
  • -Werror= various, including vla, format-security, declaration-after-statement
  • -Wdate-time
  • -Wnull-dereference
  • -Wduplicated-cond
  • -Wduplicated-branches (possibly)
  • -Wextra-semi
  • -Wcomma
  • -Wdocumentation -Wno-documentation-deprecated-sync
  • -Wunused-private-field
  • -Wnon-virtual-dtor
  • -Wold-style-cast
  • -Woverloaded-virtual
  • -Wweak-vtables
  • -Wdeprecated
  • -Wabstract-vbase-init
  • -Winconsistent-missing-destructor-override
  • -Wzero-as-null-pointer-constant

I realize that some of these may already be enabled by default, or enabled with (e.g.) -Wall, but there are so many different compiler options to be certain.

@rouault
Copy link
Member Author

rouault commented Feb 21, 2024

@mwtoews You could have a look/borrow from GDAL's gdal.cmake lines 100 to 200.
Looking at https://clang.llvm.org/docs/DiagnosticsReference.html and https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html, potential candidates could -Wcomma -Wextra-semi -Wold-style-cast -Wunused-private-field -Wzero-as-null-pointer-constant -Wshorten-64-to-32 -Wlogical-op -Wduplicated-cond -Wduplicated-branches -Wdocumentation -Wdeprecated -Woverloaded-virtual -Wdate-time -Wweak-vtables -Werror for vla , format-security and declaration-after-statement
-Winit-self is now enabled by default on gcc by -Wall.
-Wnon-virtual-dtor and -Weffc++ are synonymous with clang

@mwtoews mwtoews mentioned this pull request Feb 21, 2024
2 tasks
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.

3 participants