Always compile tests with all warnings enabled and error out on warnings

[t-b]

I think we should patch test/CMakeLists.txt to always use -Wall -Werror on *nix and the corresponding MSVC flags. It is way too easy to overlook a warning in the CI output as #1797 has shown.

@nlohmann Thoughts?

[nlohmann]

I compile the code with maximal warnings when I prepare a release. For CI, such a step would be nice - PRs welcome!

[amiremohamadi]

@nlohmann
I'll be happy if I can work on it.
I've a question! Should only related flags be added to the test/CMakeLists.txt?

[t-b]

@amiremohamadi I'm not sure what you mean by "related flags" but I would start with -Wall -Werror for gcc/clang and /W4 /WX MSVC. Adding it to test/CmakeLists.txt makes most sense.

[MBalszun]

In principle it is a nice goal, but it's always hard to retrofit -Werror / /WX into a project that runs CI on a wide variety of compilers and hasn't used those flags before.

Often you don't have all compilers locally and have to debug through CI cycles and in some cases it is also not clear how to best fix a warning. Particularly problematic: If someone wants to add a new compiler to the CI, is it that person's responsibility to fix all new warnings that that compiler spits out before the PR can be merged?

Maybe a less ambitious first step would be to ensure that the user-visible headers don't produce any warnings (e.g. create a separate test that includes the headers and not much more and compile that single test with -Werror / /WX. From that, iteratively add the flag to more and more tests. In total that is more work, but has the advantage that you can merge intermediate steps and don't get regressions from new pull requests on the way.

[nlohmann]

I can understand your point, but so far, any warning that did come up eventually lead to an issue here. And if we would be able to detect such a warning in the CI, we have the opportunity to fix it immediately rather than with a delay. That said, I'd rather have such an ambitious step and deal with the problems later than to start every second release notes with notes on fixed warnings which slipped through the release process as, for instance, I do not use MSVC myself. Does this make sense?

[MBalszun]

Sure, I haven't actually looked into how many warnings there are currently generated - I just saw that a lot of CI tests started to fail and unless I'm missing something, there is at least one false positive on Appveyor about an unused parameter that is actually used (https://ci.appveyor.com/project/nlohmann/json/builds/28915988/job/jsk75q0pso682j5a).

Luckily, the code base is in good shape and not "that" big, so I guess, fixing all warnings in one PR is manageable.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.