unused parameter warnings should not cause errors on non-release builds

[c-dilks]

Is your feature request related to a problem? Please describe. Treating all warnings as errors may not be the best approach for productivity. Sometimes I turn on/off certain parts of the code, which often triggers unused parameter warnings (treated as errors), which are tedious to resolve.

Describe the solution you'd like Consider mofifying https://github.com/eic/epic/blob/d14e80b98cc51fb7acf014f6984caf8fe347aed1/CMakeLists.txt#L26-L29

Should we be less strict for non-release builds?

[wdconinc]

In my opinion we should be strict but we can provide an override. Would this solve your use case?

# Error on all warnings 
set(DEFAULT_COMPILE_OPTIONS "-Wall -Wextra -Werror -pedantic" CACHE STRING "Default compile options")
if(NOT CMAKE_BUILD_TYPE STREQUAL "Release") 
  add_compile_options(${DEFAULT_COMPILE_OPTIONS}) 
endif() 

[c-dilks]

Yes, that would work, though -DCMAKE_BUILD_TYPE=Release also solves my use case without any changes.

I think we should be more strict for CI builds (error on all warnings) and less strict for default builds (warnings are just warnings); that way developers are just notified of warnings, but PRs with warnings aren't allowed to be merged. Why is the default build type RelWithDebInfo more strict than Release?

[wdconinc]

In my experience, developers get annoyed if their stuff fails in CI when they had ensured that it worked fine locally :-) This selection of defaults (e.g. RelWithDebInfo) was to support developers as they develop and have a need to debug. Do you want to change the default to Release and then only run with something lower in CI?

[c-dilks]

How about a Debug build type, which just keeps -Werror off?

# Error on all warnings
if(NOT CMAKE_BUILD_TYPE STREQUAL "Release")
  add_compile_options(-Wall -Wextra -pedantic)
  if(NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
    add_compile_options(-Werror)
  endif()
endif()

[wdconinc]

Isn't it exactly in a developer environment that you should want to see all warning that could cause failures later on, e.g. in CI? That's when you are in the mindset and environment where you are most likely to have introduced them and can fix them.

I kinda feel that we shouldn't change defaults because of a specific testing/development strategy (disabling part of the code), done by an experienced developer (you) where we have identified a workaround. The defaults should be to 'strongly encourage' developers to fix warnings right from the start.

But I'm just one person here and we have a disagreement, so let's maybe see what @veprbl @sly2j think about this (also since it probably is good to be consistent in some of the other code repositories).

[c-dilks]

Not setting -Werror means all warnings will still appear, they just won't be converted to errors; that is all the above proposed non-default Debug build type would do.

At first glance, I only see -Werror used for epic and juggler, so we're the odd ones out here. But there are some cases where -Werror is applied for CI jobs, e.g., ACTS.

[veprbl]

My take on -Werror is:

1. It should always be enabled on CI
2. It doesn't make sense to push it onto the end users who may have a different version of a compiler that was not tested

As far as local development goes, it probably falls close enough to apply rule 2. The remaining warnings can be brushed up once PR is submitted.