Pcl: Reduce warnings during compilation with all (usual) warnings switched on

[SunBlack]

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.

[taketwo]

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]

I agree with @taketwo on this one.

[kunaltyagi]

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

Current status per CI:

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

Current status of modules:

• Issues
  
  
  
  • [ ] Features: #3368
  
  
  • [ ] Apps: #3370
  
  
• Pull requests
  
  
  
  • [x] Common: #3374
  
  
  • [x] Segmentation: #3375
  
  
  • [x] People: #3377
  
  
  • [x] Surface/Nurbs: #3345
  
  
  • [x] Recognition/Metslib: #3384
  
  
  • [x] Tests: #3385, #3382 fixes #3378, #3425 fixes #3405, #3677 fixes #3373
  
  
  • [x] Registration: #3389 fixes #3376
  
  
  • [x] Filters: #3429 fixes #3367
  
  
  • [x] Apps: #3406 fixes #3403, #3469 fixes #1427
  
  

Other related problems(?):

• AppleClang warnings: fixed by #3777
  
  
  
  • [x] Closed: #3785
  
  
  • [x] missing field 'fst_bytesalloc' initializer: fixed by #3736
  
  
  • [x] ld: warning: text-based stub file are out of sync. Falling back to library file for linking Possible solutions on SO #3330 fixed by #3736
  
  
• ~Mismatch in flags for different compilers on the same system (GCC vs Clang on Linux)~
• ~Mismatch in flags for different compilers on different systems (GCC vs AppleClang vs MSVC) ~

[kunaltyagi]

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

[SunBlack]

MSVC does know /Wall.

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

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

[kunaltyagi]

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]

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]

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]

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

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