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

Reduce warnings during compilation with all (usual) warnings switched on #2746

Open
SunBlack opened this issue Dec 27, 2018 · 9 comments
Open
Labels
effort: medium Rough estimate of time needed to fix/implement/solve good first issue Skills/areas of expertise needed to tackle the issue kind: todo Type of issue module: cmake platform: linux platform: windows

Comments

@SunBlack
Copy link
Contributor

To increase code quality of new PR, we should treat warnings as errors (as already mentioned here #2733).

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Werror")

In case we have not solved all compiler warnings, before introducing this change, we have to whitelist this warning types (global or per target).

Because with new compilers sometimes new compiler warnings will be introduced, we should care about this:

  • Option 1: Instead of -Werror explicitly list all warning types which should treat as error (this list could be really long), so new warnings will be not treated as error
  • Option 2: Add an option to enable/disable this change.
if(CMAKE_COMPILER_IS_CLANG OR CMAKE_COMPILER_IS_GCC)
  option(PCL_Treat_warnings_as_errors TRUE) # TRUE or FALSE as default?
  if(PCL_Treat_warnings_as_errors )
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Werror")
  endif()
endif()

Before we can introduce this, we need to resolve #2732 & #2745, so 3rd-party code cannot raise warnings.

@SunBlack SunBlack mentioned this issue Dec 27, 2018
@taketwo
Copy link
Member

taketwo commented Dec 28, 2018

Related: #2669 (discusses which warnings to enable).

I think treating warnings as errors should be an opt-in. Disabled by default, but enabled in CI pipelines. As usual, I would look at OpenCV scripts to check how they handled this.

@SergioRAgostinho
Copy link
Member

I agree with @taketwo on this one.

@kunaltyagi
Copy link
Member

kunaltyagi commented Apr 23, 2020

Some commits have been already merged. This will track other warnings based on issues/PR

Current status per CI:

  • Format
  • MacOS Mojave
  • MacOS Catalina
  • Ubuntu 16.04
  • Ubuntu 19.10
  • Windows 32-bit
  • Windows 64-bit
  • Documentation: WIP by @aPonza

Current status of modules:

Other related problems(?):

@kunaltyagi
Copy link
Member

kunaltyagi commented Apr 23, 2020

Aiming at reduction in windows warnings, I counted them:

Warning Count left (without tests) Count left (in tests) Reason PRs
C4018 1 2 signed unsigned mismatch
C4068 690 0 3 unknown pragma #3964
C4101 13 1 unreferenced local variable
C4146 5 0 unary minus operator applied to unsigned type, result still unsigned
LNK4199 5 0 dll not strictly needed
C4244 208 191 narrowing (eg: double to float) #3967
C4316 9 3 heap allocated object might not be aligned_as(16-bit)
C4477 6 0 printf format string and variable don't match
C4661 12257 165 no suitable definition provided for explicit template instantiation request
C4804 0 12 unsafe use of bool
C4996 300 54 use of deprecated data member

And other warnings from tests only (lower priority).
Script used:

# edit before running
curl https://dev.azure.com/PointCloudLibrary/0fc52e87-00b9-420e-acbc-c842c4f2d9cc/_apis/build/builds/15904/logs/80 | \
  grep warning | grep [-v] test | grep <warning> | wc -l

Some of these are good warnings for beginners to tackle (like 4477, 4244, 4146, etc.). Others not so much.

I don't know if /Wall is switched on for Windows CI

@kunaltyagi kunaltyagi added good first issue Skills/areas of expertise needed to tackle the issue kind: todo Type of issue platform: linux platform: windows effort: medium Rough estimate of time needed to fix/implement/solve and removed kind: proposal Type of issue labels Apr 23, 2020
@kunaltyagi kunaltyagi changed the title Treat warnings as errors. Reduce warnings during compilation Apr 23, 2020
@kunaltyagi kunaltyagi changed the title Reduce warnings during compilation Reduce warnings during compilation with all (usual) warnings switched on Apr 23, 2020
@SunBlack
Copy link
Contributor Author

SunBlack commented Apr 24, 2020

MSVC does know /Wall.
image

To treat as warnings as errors you have to set flag /WX
image

See also MSVC documentation for these flags and list of all warnings and their level.

@kunaltyagi
Copy link
Member

MSVC does know /Wall

I was wondering if we set it on the CI because IIRC, windows flags are set in CMake. If no, then the warnings will only increase

@SunBlack
Copy link
Contributor Author

As far as I know you cannot enable/disable single warnings on MSVC. As we have already enough warnings, I think current level is okay. So maybe we should start to enable /WX /W3 before we switch to /WX /Wall

@SergioRAgostinho
Copy link
Member

As far as I know you cannot enable/disable single warnings on MSVC.

I found some references before that suggest otherwise.

Is it not better to actually, pass to MSVC an explicit global option to ignore unknown pragmas. This is something we definitely want to have enabled globally.

Edit: According to this /wdnnnn where nnnn is the warning code.

@SunBlack
Copy link
Contributor Author

Edit: According to this /wdnnnn where nnnn is the warning code.

Ahh good to know (should have read my link completly myself 😆)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium Rough estimate of time needed to fix/implement/solve good first issue Skills/areas of expertise needed to tackle the issue kind: todo Type of issue module: cmake platform: linux platform: windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants