snitch-org / snitch

Lightweight C++20 testing framework.
Boost Software License 1.0
257 stars 7 forks source link

Check your code with werror and wpedantic #77

Closed victimsnino closed 1 year ago

victimsnino commented 1 year ago

Hey! Thank you for this amazing testing framework. Currently I’m trying to add it to my library, but CI gets a bunch of error on your library due to werror and wpedantic flags

for example Error: /home/runner/work/ReactivePlusPlus/ReactivePlusPlus/build/_deps/snitch-src/src/snitch.cpp:76:55: error: format string is not a string literal [-Werror,-Wformat-nonliteral] 60 const int return_code = std::snprintf(nullptr, 0, get_format_code(), value);

Or some stupid one

Error: /home/runner/work/ReactivePlusPlus/ReactivePlusPlus/build/_deps/snitch-src/include/snitch/snitch.hpp:1151:2: error: extra ‘;’ [-Werror=pedantic] 66 1151 | }; // namespace event

Full run can be found there: https://github.com/victimsnino/ReactivePlusPlus/actions/runs/4536738324

I could find workaround, but anyway it is better for you to have zero warnings even on most aggressive level :)

cschreib commented 1 year ago

Hi there and thank you for the bug report. I am a bit confused as to why I am not catching these warnings, because I do compile the code with -Wall -Wextra -Werror. I don't use -pedantic, but I just tried adding it to the compilation options, and I am still not getting the warnings you are seeing. In case it was something wrong with my CMake scripts, I just copied snitch_all.hpp into Godbolt and explicitly set these warning levels, both with the latest clang and gcc; still no error.

Are you using a specific compiler version, and/or a specific platform? Or perhaps some additional compilation options. Are you able to create a minimal example I can reproduce, outside of your CI scripts? Having the full compilation command would also help.

At any rate, the warning about the extra semicolon is genuine, and I will fix it. The other warning is noise; I may be able to find an alternative implementation that does not trigger it, but that first requires me being able to reproduce it.

victimsnino commented 1 year ago

I'm catching these errors during linking against snitch::snitch target, not header-only version. Most of the errors came from snitch.cpp file. Try this one. If you still can't reproduce, i would try to prepare example

cschreib commented 1 year ago
g++-10 -std=c++20 -Wall -Wextra -Werror -pedantic snitch.cpp -o snitch.o -I ../include -I ../build/

Still nothing :shrug:

cschreib commented 1 year ago

Correction, I do get

../include/snitch/snitch.hpp:1701:2: error: extra ‘;’ [-Werror=pedantic]
 1701 | }; // namespace event
      |  ^

I had already fixed it on my branch, hence why it wasn't showing up... Weirdly, it shows up only when compiling with g++-10, and not g++-11! Anyway, that one will be fixed.

For the rest of the warnings, I looked at your CI scripts, you specify more warnings on the command line: -Wall -Werror -Wextra -Wpedantic -Wcast-qual -Wformat=2 -Wundef -Werror=float-equal. If I use this, then I get

In file included from snitch.cpp:1:
../include/snitch/snitch.hpp: In function ‘constexpr snitch::impl::float_bits snitch::impl::to_bits(float)’:
../include/snitch/snitch.hpp:647:11: error: comparing floating-point with ‘==’ or ‘!=’ is unsafe [-Werror=float-equal]
  647 |     if (f != f) {
      |         ~~^~~~
../include/snitch/snitch.hpp:652:18: error: comparing floating-point with ‘==’ or ‘!=’ is unsafe [-Werror=float-equal]
  652 |     } else if (f == std::numeric_limits<float>::infinity()) {
      |                ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../include/snitch/snitch.hpp:657:18: error: comparing floating-point with ‘==’ or ‘!=’ is unsafe [-Werror=float-equal]
  657 |     } else if (f == -std::numeric_limits<float>::infinity()) {
      |                ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
snitch.cpp: In instantiation of ‘bool {anonymous}::append_fmt(snitch::small_string_span, T) [with T = const void*; snitch::small_string_span = snitch::small_vector_span<char>]’:
snitch.cpp:119:34:   required from here
snitch.cpp:76:42: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
   76 |     const int return_code = std::snprintf(nullptr, 0, get_format_code<T>(), value);
      |                             ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
snitch.cpp: In instantiation of ‘bool {anonymous}::append_fmt(snitch::small_string_span, T) [with T = long unsigned int; snitch::small_string_span = snitch::small_vector_span<char>]’:
snitch.cpp:124:28:   required from here
snitch.cpp:76:42: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
snitch.cpp: In instantiation of ‘bool {anonymous}::append_fmt(snitch::small_string_span, T) [with T = long int; snitch::small_string_span = snitch::small_vector_span<char>]’:
snitch.cpp:128:28:   required from here
snitch.cpp:76:42: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
snitch.cpp: In instantiation of ‘bool {anonymous}::append_fmt(snitch::small_string_span, T) [with T = float; snitch::small_string_span = snitch::small_vector_span<char>]’:
snitch.cpp:132:28:   required from here
snitch.cpp:76:42: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
snitch.cpp: In instantiation of ‘bool {anonymous}::append_fmt(snitch::small_string_span, T) [with T = double; snitch::small_string_span = snitch::small_vector_span<char>]’:
snitch.cpp:136:28:   required from here
snitch.cpp:76:42: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]

Seems like they are coming from -Wformat=2 and -Werror=float-equal. These aren't included in -Wall -Wextra -pedantic, which is already quite a conservative set of warnings. If you want to enable these warnings, would suggest silencing them when building/including snitch. I'm afraid I can't support any and all combinations of warnings, particularly the non-standard ones.

victimsnino commented 1 year ago

yeah, sure, no problem. It is not blocker for me, just wanted to notify you about possible issue :). In this case I would suggest you to add (or at least, to mention)

target_compile_options(snitch PRIVATE "-w")

as part of the "installation guide"