search

celtera / libremidi

A modern C++ MIDI 1 / MIDI 2 real-time & file I/O library. Supports Windows, macOS, Linux and WebMIDI.
Other
456 stars 51 forks source link

Fixes (most) -Wall and -Wextra warnings #93

Closed jcelerier closed 10 months ago

jcelerier commented 10 months ago

@lilggamegenius there were a lot of build errors with the PR actually ^^' let's put the work to fix them here

jcelerier commented 10 months ago

hmm and I'm still getting -Werror leaking in downstream projects, investigating why...

jcelerier commented 10 months ago

okay, problems can come up if the user has some additional warnings - e.g. I have -Wnon-virtual-dtor in a downstream project as CMAKE_CXX_FLAGS and thus this breaks the build. I'm going to enable Werror only during CI this way it won't break randomly for users.

lilggamegenius commented 10 months ago

I think I may have figured out why the warnings are leaking through downstream. I think it might be this in the cmake file

target_include_directories(libremidi ${_public}
  $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
  $<INSTALL_INTERFACE:include>
)

Specifically, I think before ${_public}, It needs to have SYSTEM to tell cmake to mark the includes as system directories so that warnings are suppressed when used downstream. You could add a new _system variable that gets set to SYSTEM normally, but is empty for CI. I haven't tested this yet but I managed to get it working by doing this which is basically the downstream side way of doing this.

lilggamegenius commented 10 months ago

The other target_include_directories should probably also have SYSTEM added, but not using the _system variable since those are actual external dependencies.