Skip to content

Conversation

@skaravos
Copy link
Contributor

@skaravos skaravos commented Nov 5, 2024

  • Provides a workaround for #249

  • Removes -Werror from the default set of compiler warnings for GCC to lessen the impact on downstream package maintenance.

  • Adds two new cmake options:

    1. LOGURU_ENABLE_COMPILER_WARNINGS

      • enabled by default when the project is compiled as the top-level cmake project,
        can be explicitly set to FALSE to opt-out of compiler warnings
    2. LOGURU_WARNINGS_AS_ERRORS

      • disabled by default, can be explicitly set to TRUE to opt-in to treating warnings as errors

Changes:
- Provides a workaround for
  [emilk#249](emilk#249)
- Adds an option that can be used to disable compile warnings for
  the loguru library. This could be useful for users who just want to
  build the library and dont care about the warnings.
  (or they just aren't in a position to fix them anyways).

Notes:
- The default behaviour remains the same: compiler-warnings are enabled
  by default when the project is built as a top-level project, and
  disabled by default when the project is included as a sub-project with
  either add_subdirectory() or FetchContent()
Notes:
- This is needed to quiet a cmake warning about non-cache variables
  being used to set loguru cmake `options()`, because loguru's minimum
  cmake version is still 3.10 and this wasn't allowed until cmake 3.13.

  ```
  # CMakeLists.txt

  # prior to cmake 3.13 the user would need to set options as cache vars
  set(LOGURU_ENABLE_COMPILER_WARNINGS FALSE CACHE BOOL "" FORCE)

  # cmake 3.13 and onwards the sub-project options respect normal vars.
  set(LOGURU_ENABLE_COMPILER_WARNINGS FALSE)

  add_subdirectory(loguru)
  ```
Changes:
- Provides a workaround for [emilk#249](emilk#249)
- Removed `-Werror` from the default compiler warnings for GCC.
- Added an option that can be used to opt-in to warnings-as-errors for
  all three supported compilers (gcc, clang, msvc) if desired.
@drupol
Copy link

drupol commented Nov 6, 2024

Just for my own information, what's the technical difference between this method and fixing the issue in the CPP code directly? Like these 2 patches are doing (https://github.com/virtuosonic/loguru/commit/e1ffdc4149083cc221d44b666a0f7e3ec4a87259.patch and https://github.com/virtuosonic/loguru/commit/743777bea361642349d4673e6a0a55912849c14f.patch) ?

@skaravos
Copy link
Contributor Author

skaravos commented Nov 7, 2024

Just for my own information, what's the technical difference between this method and fixing the issue in the CPP code directly? Like these 2 patches are doing (https://github.com/virtuosonic/loguru/commit/e1ffdc4149083cc221d44b666a0f7e3ec4a87259.patch and https://github.com/virtuosonic/loguru/commit/743777bea361642349d4673e6a0a55912849c14f.patch) ?

This PR addresses a different concern entirely.
You're right in pointing out that the proper fix to #249 is applying those two patches to the C++ code, and it would certainly be worth having a PR's submitted for those.

However, even if the C++ code gets fixed it doesn't address the fact that the default out-of-the-box CMake build for the project should not have -Werror enabled in the first place.
I didn't really even mean to have -Werror added to the CMake file (as evidenced by the fact I only had the flag enabled for GCC, but not clang or MSVC) it was an oversight and this PR addresses that.

Notes:
- Realized I must've dropped the actual most important commit while
  rebasing this branch before submission, sheesh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants