skypjack / uvw

Header-only, event based, tiny and easy to use libuv wrapper in modern C++ - now available as also shared/static library!
MIT License
1.82k stars 207 forks source link

suppress compiler warning #294

Closed falsycat closed 1 year ago

falsycat commented 1 year ago

(sry for poor english)

I'm using uvw v3.2.0_libuv_v1.46 and want a way to suppress compiler warnings from header files of uvw because they're annoying.

I think it's the easiest solution to add SYSTEM keyword to target_include_directories in CMakeLists.txt. I'll make PR if agreed.

I know the best one is fixing all warnings, however, doing that for all compilers will cost too much, I think.

thank you

skypjack commented 1 year ago

What warnings exactly? I don't see them on the CI and I can't reproduce them without a bit of context.

falsycat commented 1 year ago

@skypjack

thank you for your reply!

warning:

/Users/falsycat/Repo/nf7/build/_deps/uvw-src/src/uvw/uv_type.hpp:19:34: error: unused parameter 'token' [-Werror=unused-parameter]
   19 |     explicit uv_type(loop::token token, std::shared_ptr<loop> ref) noexcept
      |                      ~~~~~~~~~~~~^~~~~

full compile command displayed by make VERBOSE=1:

cd /Users/falsycat/Repo/nf7/build/core && /opt/homebrew/bin/g++-13 -DUVW_AS_LIB -I/Users/falsycat/Repo/nf7/build -I/Users/falsycat/Repo/nf7 -I/Users/falsycat/Repo/nf7/build/_deps/uvw-src/src -I/Users/falsycat/Repo/nf7/build/_deps/libuv-src/include -isystem /Users/falsycat/Repo/nf7/build/_deps/luajit-src/src -g -std=gnu++23 -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX13.3.sdk -mmacosx-version-min=11.0 -Wall -Wextra -Wpedantic -Werror -MD -MT core/CMakeFiles/nf7_core.dir/uv/concurrency.cc.o -MF CMakeFiles/nf7_core.dir/uv/concurrency.cc.o.d -o CMakeFiles/nf7_core.dir/uv/concurrency.cc.o -c /Users/falsycat/Repo/nf7/core/uv/concurrency.cc

CMake script about uvw:

FetchContent_Declare(
  uvw
  GIT_REPOSITORY https://github.com/skypjack/uvw.git
  GIT_TAG        v3.2.0_libuv_v1.46
)
set(BUILD_UVW_LIBS ON)
FetchContent_MakeAvailable(uvw)

compiler version:

$ g++-13 --version
g++-13 (Homebrew GCC 13.1.0) 13.1.0
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

the warning can be suppressed by adding SYSTEM after ${LIB_NAME} in the following code: https://github.com/skypjack/uvw/blob/7fda7c5072cdc1b78fc62b1cb1f2842e465cb02b/src/CMakeLists.txt#L38-L43

skypjack commented 1 year ago

Is this the only warning you have? Fixing it in code looks both reasonable and trivial in any case.

falsycat commented 1 year ago

@skypjack

I know it's the best, however, I think thirdparty libraries shouldn't make a warning to reduce a time of library user to deal with new warnings caused by compiler differences, like this issue.

SYSTEM keyword seems to suppress warnings only when the headers are included by #include <...> (not #include "..."). So this solution doesn't affect to developers unless using #include <..>.

(that warning was all I saw)

aloisklink commented 1 year ago

Is this the only warning you have?

I'm having a couple of other warnings too.

I'm using the following compiler warnings (these are a subset of the default compiler flags from the cmake-init project, see https://github.com/friendlyanon/cmake-init/blob/aa42211c79ab5117b05a2d9f795427f078a0a3d5/cmake-init/templates/common/CMakePresets.json#L88):

add_compile_options(
  # Error on all C/C++ warnings if making a Debug build
  $<$<CONFIG:Debug>:-Werror>
  -Wall
  -Wextra
  -Wpedantic
  -Wconversion
  -Wsign-conversion
  -Wcast-qual
  -Wformat=2
  -Wundef
  -Werror=float-equal
  -Wshadow
  -Wcast-align
  -Wunused
  -Wnull-dereference
  -Wdouble-promotion
  -Wimplicit-fallthrough
  $<$<COMPILE_LANGUAGE:CXX>:-Wextra-semi>
  $<$<COMPILE_LANGUAGE:CXX>:-Woverloaded-virtual>
  $<$<COMPILE_LANGUAGE:CXX>:-Wnon-virtual-dtor>
  $<$<COMPILE_LANGUAGE:CXX>:-Wold-style-cast>
)

Most of the warnings look pretty minor/trivial to fix (I might even make a PR if I have free time this weekend!).

skypjack commented 1 year ago

I'm having a couple of other warnings too.

@aloisklink I'm open to look into a PR if you like. Otherwise, I can try this list and fix them when I'm back on full bandwidth (that is next week probably, sort of at least).

aloisklink commented 1 year ago

Otherwise, I can try this list and fix them when I'm back on full bandwidth (that is next week probably, sort of at least).

Sounds good! I'll give it a try if I find the time. Fixing the warnings should be the easy part, the hard part will be enabling all of the warnings in CI, but only for compilers that support them, in order to prevent future PRs from adding new warnings.


By the way, @falsycat, CMake 3.25 added support for the SYSTEM property on directories and targets.

I'm not sure how you're using uvw, but if you're using CPM, you can do something like:

CPMAddPackage(uvw
  NAME uvw
  VERSION 3.2.0
  SYSTEM YES # this is the important line!!
  uvw
  URL https://github.com/skypjack/uvw/archive/refs/tags/v3.2.0_libuv_v1.46.tar.gz
  URL_HASH SHA3_256=0b82039ac6fcff490caeb85b77408eb925146690cb812c4e9303757263d8ce12
  EXCLUDE_FROM_ALL YES # to avoid building uvw tests/installs
)

Or if you're using FetchContent, something like:

FetchContent_Declare(uvw
  GIT_REPOSITORY https://github.com/skypjack/uvw.git
  GIT_TAG 7fda7c5072cdc1b78fc62b1cb1f2842e465cb02b # v3.2.0_libuv_v1.46
  SYSTEM # this is the important line!!
)
FetchContent_MakeAvailable(uvw)
falsycat commented 1 year ago

Or if you're using FetchContent, something like:

FetchContent_Declare(uvw
  GIT_REPOSITORY https://github.com/skypjack/uvw.git
  GIT_TAG 7fda7c5072cdc1b78fc62b1cb1f2842e465cb02b # v3.2.0_libuv_v1.46
  SYSTEM # this is the important line!!
)
FetchContent_MakeAvailable(uvw)

I didn't know that! Thank you!