Enable and fix warnings

[ahans]

This enables the commonly used warning flags for msvc/gcc/clang and fixes the warnings reported.

[nachovizzo]

Wow thanks a lot! Some comments, and ready to merge!

[nachovizzo]

@ahans the windows build is failing, probably cuz MSVC is too annoyting with warnings, don't worry. I will take over this one ;)

[ahans]

@ahans the windows build is failing, probably cuz MSVC is too annoyting with warnings, don't worry. I will take over this one ;)

Interesting, it was working for me. But I was using a newer version, maybe they changed something there.

[nachovizzo]

@ahans the windows build is failing, probably cuz MSVC is too annoyting with warnings, don't worry. I will take over this one ;)

Interesting, it was working for me. But I was using a newer version, maybe they changed something there.

Which version? The problem seems to be that is not detecting the system headers as headers 🤔

[ahans]

@ahans the windows build is failing, probably cuz MSVC is too annoyting with warnings, don't worry. I will take over this one ;)

Interesting, it was working for me. But I was using a newer version, maybe they changed something there.

Which version? The problem seems to be that is not detecting the system headers as headers thinking

It was the latest, which is probably some 2022 one. But it's very possible I missed something.

[nachovizzo]

@ahans the windows build is failing, probably cuz MSVC is too annoyting with warnings, don't worry. I will take over this one ;)

Interesting, it was working for me. But I was using a newer version, maybe they changed something there.

Which version? The problem seems to be that is not detecting the system headers as headers thinking

It was the latest, which is probably some 2022 one. But it's very possible I missed something.

Turns out that is the MSVC version of the gh actions runners/dockers, whatever is in there. I've checked and it's version 17.x and I found this resource online: https://discourse.cmake.org/t/marking-headers-as-system-does-not-suppress-warnings-on-windows/6415 that says at least version 19

[ahans]

Turns out that is the MSVC version of the gh actions runners/dockers, whatever is in there. I've checked and it's version 17.x and I found this resource online: https://discourse.cmake.org/t/marking-headers-as-system-does-not-suppress-warnings-on-windows/6415 that says at least version 19

That link says you need to upgrade CMake. And some GH documentation says windows-2022 has VS 2022. So this should be fine actually. Please don't merge this like this, we can do this proper. I can have a closer look tomorrow.

[nachovizzo]

Turns out that is the MSVC version of the gh actions runners/dockers, whatever is in there. I've checked and it's version 17.x and I found this resource online: https://discourse.cmake.org/t/marking-headers-as-system-does-not-suppress-warnings-on-windows/6415 that says at least version 19

That link says you need to upgrade CMake. And some GH documentation says windows-2022 has VS 2022. So this should be fine actually. Please don't merge this like this, we can do this proper. I can have a closer look tomorrow.

On the last runs:

1s
Current runner version: '2.303.0'
Operating System
  Microsoft Windows Server 2022
  [1](https://github.com/PRBonn/kiss-icp/actions/runs/4536893530/jobs/7994133538#step:1:1)0.0.[2](https://github.com/PRBonn/kiss-icp/actions/runs/4536893530/jobs/7994133538#step:1:2)0[3](https://github.com/PRBonn/kiss-icp/actions/runs/4536893530/jobs/7994133538#step:1:3)[4](https://github.com/PRBonn/kiss-icp/actions/runs/4536893530/jobs/7994133538#step:1:4)8
  Datacenter

and

-- The CXX compiler identification is MSVC 19.34.31942.0

Nevertheless, it does fail anyway :S

[ahans]

Nevertheless, it does fail anyway :S

Yes, because you need to upgrade CMake to properly. That's at least what they say here:

Ignoring warnings from system headers is only supported with MSVC with the Ninja generators as of CMake 3.22 and the Visual Studio generators as of CMake 3.24

Let me quickly try that.

[nachovizzo]

Nevertheless, it does fail anyway :S

Yes, because you need to upgrade CMake to properly. That's at least what they say here:

Ignoring warnings from system headers is only supported with MSVC with the Ninja generators as of CMake 3.22 and the Visual Studio generators as of CMake 3.24

Let me quickly try that.

Or use ninja, it does not fail in the windows-python-package because it is using ninja anywyas

[ahans]

Nevertheless, it does fail anyway :S

Yes, because you need to upgrade CMake to properly. That's at least what they say here:

Ignoring warnings from system headers is only supported with MSVC with the Ninja generators as of CMake 3.22 and the Visual Studio generators as of CMake 3.24

Let me quickly try that.

Or use ninja, it does not fail in the windows-python-package because it is using ninja anywyas

The CMake upgrade did the trick. Anyways, keep whatever you want, we have some options now.

[nachovizzo]

Nevertheless, it does fail anyway :S

Yes, because you need to upgrade CMake to properly. That's at least what they say here:

Ignoring warnings from system headers is only supported with MSVC with the Ninja generators as of CMake 3.22 and the Visual Studio generators as of CMake 3.24

Let me quickly try that.

Or use ninja, it does not fail in the windows-python-package because it is using ninja anywyas

The CMake upgrade did the trick. Anyways, keep whatever you want, we have some options now.

I tried some minutes the ninja thing and didn't work out great in my case... so dropping this for now. I will just change the cmake version to 3.24 since this will match the requirements of the project, and I don't want to change that now.

Thanks for the effort and the guidance ;)

[ahans]

I squashed and cleaned up the commits. The one where you introduce the range-based loop I left intact, since it has nothing to do with warning-removal. It's good to merge from my point of view. The error for the macOS build is hopefully something transitory, so simply retriggering should do the trick. I didn't touch this at all.

[nachovizzo]

• Fix ROS1 Melodic build

[ahans]

• Fix ROS1 Melodic build

Not sure what you mean. It seems to be building fine for me. Maybe merge the other PR first and then we rebase this and I can see what CI has to say about it.

[nachovizzo]

• Fix ROS1 Melodic build

Not sure what you mean. It seems to be building fine for me. Maybe merge the other PR first and then we rebase this and I can see what CI has to say about it.

was a note for myself, actually. You can test the build in a dockerize environment using gitlab-ci-local ros1_melodic on the root repo (while we finish porting from gitlab CI to github CI)

Tool:https://github.com/firecow/gitlab-ci-local

[ahans]

• Fix ROS1 Melodic build

Not sure what you mean. It seems to be building fine for me. Maybe merge the other PR first and then we rebase this and I can see what CI has to say about it.

was a note for myself, actually. You can test the build in a dockerize environment using gitlab-ci-local ros1_melodic on the root repo (while we finish porting from gitlab CI to github CI)

That's a really cool tool. And you were right, the melodic build was broken. I thought I had looked at that manually (basically running manually what is specified in the .gitlab-ci.yml), but that was not melodic, but noetic. And melodic was only broken due to the ancient compiler. It still has a bug that reports unused variables when it shouldn't. So sorry for removing your (void) voxel fix. It's unfortunately necessary if bionic with its default compiler is to be supported. I restored it now.

[nachovizzo]

• Fix ROS1 Melodic build

Not sure what you mean. It seems to be building fine for me. Maybe merge the other PR first and then we rebase this and I can see what CI has to say about it.

was a note for myself, actually. You can test the build in a dockerize environment using gitlab-ci-local ros1_melodic on the root repo (while we finish porting from gitlab CI to github CI)

That's a really cool tool. And you were right, the melodic build was broken. I thought I had looked at that manually (basically running manually what is specified in the .gitlab-ci.yml), but that was not melodic, but noetic. And melodic was only broken due to the ancient compiler. It still has a bug that reports unused variables when it shouldn't. So sorry for removing your (void) voxel fix. It's unfortunately necessary if bionic with its default compiler is to be supported. I restored it now.

Yup, it sucks to provide compatibility to such old tools. But people already asked for ROS Melodic (#53, #78)

gitlab-ci-local was a game changer for me, specially because my desktop is much faster than our gitlab servers, for github actions I use act