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:

• 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:

• Issues
  • Features: memset for Eigen::Matrix in integral_image2D.cpp #3368
  • Apps: QVTKWidget deprecated (in VTK >= 8.2 at least) #3370
• Pull requests
  • Common: Removes deprecated usage of getFields(cloud, fields) #3374
  • Segmentation: Reordered initializers in the correct order in `supervoxel_clus… #3375
  • People: Removes warnings about multi-line macro and not covered by if #3377
  • Surface/Nurbs: Suppress Nurbs warnings #3345
  • Recognition/Metslib: Remove deprecated throw specifier #3384
  • Tests: Fix pragma declaration for CI gcc, use "-Wdeprecated-declaratio… #3385, Move OMP version of MovingLeastSquares into a separate test case #3382 fixes Ignore deprecated use for testing surface::MovingLeastSquares #3378, Fix copy constructor warning when initializing CloudView #3425 fixes Warning regarding copy ctor in cloud_view.cpp #3405, Update unit tests to support both old and new GTest versions #3677 fixes GTest deprecated TYPED_TEST_CASE #3373
  • Registration: Fix the OpenMP errors/warnings in "ia_fpcs.hpp" #3389 fixes OMP compilation error in registration test #3376
  • Filters: Fix incorrect switch fallthrough in convolution operator #3429 fixes Implicit fallthrough warning in convolution.hpp #3367
  • Apps: Do not use deprecated function in stereo_ground_segmentation #3406 fixes Use of deprecated feature (extra unneeded args) in pcl_stereo_ground_segmentation #3403, Remove SupervoxelClustering::getColoredVoxelCloud() #3469 fixes Deprecated function getColoredVoxelCloud() is used in cloud composer #1427

Other related problems(?):

• AppleClang warnings: fixed by Fix sign-compare warnings issued by AppleClang #3777
  • Closed: Remove last few warnings left on MacOS CI #3785
  • missing field 'fst_bytesalloc' initializer: fixed by Add Catalina and Xcode toolchain for MacOS CI #3736
  • ld: warning: text-based stub file are out of sync. Falling back to library file for linking Possible solutions on SO Export SDK version of MacOS to prevent some linker warnings #3330 fixed by Add Catalina and Xcode toolchain for MacOS CI #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 😆)