CMake: Enable -Werror=unused-parameter flag to treat unused-parameter warnings as errors

[VasuDevrani]

for #2266
this will output unused-parameters warnings as errors.

[torwig]

@VasuDevrani Should we also fix those errors in the scope of this PR?

[VasuDevrani]

@VasuDevrani Should we also fix those errors in the scope of this PR?

maybe yes (but they are a lot), I think the CI failing because of those.

[PragmaTwice]

Yeah we should fix them otherwise the PR will be blocked by CI failure.

You can use the attribute [[maybe_unused]].

[VasuDevrani]

You can use the attribute [[maybe_unused]].

Wouldn't it be better to drop the unused parameter and keep the type only

virtual Status ToNumber(int64_t*) const { return {Status::NotOK, "not supported"}; }
virtual Status ToBool(bool*) const { return {Status::NotOK, "not supported"}; }

[mapleFU]

Wouldn't it be better to drop the unused parameter and keep the type only

It's also ok here but keeping a name might be more specific?

[PragmaTwice]

You can use the attribute [[maybe_unused]].

Wouldn't it be better to drop the unused parameter and keep the type only

virtual Status ToNumber(int64_t*) const { return {Status::NotOK, "not supported"}; }
virtual Status ToBool(bool*) const { return {Status::NotOK, "not supported"}; }

There's some information in the names, so we'd better to keep them.

[VasuDevrani]

Two more concerns, then I'll move forward with [[maybe_unused]]:

• it can make code a bit unreadable (especially if there's multiple unused params together)

void performOperation(const Config& config, [[maybe_unused]] int param1, [[maybe_unused]] double param2, [[maybe_unused]] const std::string& param3) {
    // Use only the 'config' parameter
    if (config.isEnabled()) {
        // Perform the operation
        // ...
    }
}

VS

void performOperation(const Config& config, int, double, const std::string&) {
    // Use only the 'config' parameter
    if (config.isEnabled()) {
        // Perform the operation
        // ...
    }
}

• supported for cpp 17+ (then again project cmake sets it at 17)

[mapleFU]

We already uses C++17, yeah?

[VasuDevrani]

We already uses C++17, isn't we?

yes

[mkmkme]

Setting CMAKE_CXX_FLAGS globally is an anti-pattern in CMake.
Maybe the same effect can be achieved with

target_compile_options(kvrocks_objs PUBLIC -Werror=unused-parameter)