Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

maint: Enable -Werror compiler flag for GCC, Clang and AppleClang #3611

Merged
merged 19 commits into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ option(BUILD_LIBMAMBA_TESTS "Build libmamba C++ tests" OFF)
option(BUILD_MAMBA "Build mamba" OFF)
option(BUILD_MICROMAMBA "Build micromamba" OFF)
option(BUILD_MAMBA_PACKAGE "Build mamba package utility" OFF)
option(MAMBA_WARNING_AS_ERROR "Treat compiler warnings as errors" OFF)
if(MSVC)
option(MAMBA_WARNING_AS_ERROR "Treat compiler warnings as errors" OFF)
else()
option(MAMBA_WARNING_AS_ERROR "Treat compiler warnings as errors" ON)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good idea even if this will probably will make some untested toolchain fail.

endif()
set(
MAMBA_LTO
"Default"
Expand Down
16 changes: 16 additions & 0 deletions libmamba/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,22 @@ set(
${LIBMAMBA_SOURCE_DIR}/api/shell.cpp
${LIBMAMBA_SOURCE_DIR}/api/update.cpp
)
# TODO: remove when switch to C++20
if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang")
# This file uses capturing structured bindings, which was fixed in C++20
set_source_files_properties(
${LIBMAMBA_SOURCE_DIR}/download/mirror_impl.cpp
PROPERTIES COMPILE_FLAGS -Wno-c++20-extensions
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is nothing wrong with our code and nothing to change there. So we can safely ignore the warning we're using C++20 feature (capturing structured binding).
This can't hide any real problems, so here this warning/error will disappear.

Copy link
Member

@JohanMabille JohanMabille Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, this construction is C++20, not C++17. It works with gcc and msvc though, and the warning is still here with Clang, even if we pass the C++20 option (that's a known bug), so I agree that we can ignore it.

)
endif()

if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
# GCC reports dangling reference when using std::invoke with data members
set_source_files_properties(
${LIBMAMBA_SOURCE_DIR}/solver/problems_graph.cpp
PROPERTIES COMPILE_FLAGS -Wno-error=dangling-reference
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be useful warning/error, so I'm turning it back to being warning for this file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to make that change as part of the source code instead of here (like disabling warnings)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, fortunately it happens in functions located near each other, so we can only write pragmas once.
Thanks!

)
endif()

foreach(script ${SHELL_SCRIPTS})
list(APPEND LIBMAMBA_SOURCES shell_scripts/${script}.cpp)
Expand Down
7 changes: 7 additions & 0 deletions libmamba/ext/solv-cpp/src/pool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,17 @@ namespace solv

namespace
{
// This function is only used in `assert()` expressions
// That's why it might get reported as unused in Release builds
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-function"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explained above why this happens.
It's completely ok to have a function which is only used in assert statements.
So, I silence this warning/error


auto is_reldep(::Id id) -> bool
{
return ISRELDEP(static_cast<std::make_unsigned_t<::Id>>(id)) != 0;
}

#pragma GCC diagnostic pop
}

auto ObjPoolView::get_string(StringId id) const -> std::string_view
Expand Down
4 changes: 1 addition & 3 deletions libmamba/include/mamba/util/conditional.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,7 @@ namespace mamba::util
}
else
{
using int_t = Int;
return static_cast<int_t>(condition) * true_val
+ (int_t(1) - static_cast<int_t>(condition)) * false_val;
return condition ? true_val : false_val;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is significantly easier to read and doesn't deal with arithmetics at all (which was giving warnings in some cases).

}
}
}
Expand Down
4 changes: 2 additions & 2 deletions libmamba/src/core/activation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -554,11 +554,11 @@ namespace mamba
auto conda_environment_env_vars = get_environment_vars(prefix);

// TODO check with conda if that's really what's supposed to happen ...
std::remove_if(
void(std::remove_if(
mathbunnyru marked this conversation as resolved.
Show resolved Hide resolved
conda_environment_env_vars.begin(),
conda_environment_env_vars.end(),
[](auto& el) { return el.second == CONDA_ENV_VARS_UNSET_VAR; }
);
));

std::vector<std::string> clobbering_env_vars;
for (auto& env_var : conda_environment_env_vars)
Expand Down
26 changes: 20 additions & 6 deletions libmamba/src/core/thread_utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,25 @@ namespace mamba
#ifndef _WIN32
namespace
{
// `sigaddset` might be implemented as a macro calling `__sigbits(int)` function
// At the same time `sigset_t` might be `unsigned int`
// This causes compiler warning
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wsign-conversion"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comment explaining why this happens above
This doesn't happen in our code, but in the system code
But because sigaddset might be implemented as a macro, it's treated as our code.

Because, there is no bug, I completely disable this warning/error.
But I didn't want to disable it for the whole file, so I created a small function, where I applied this behaviour.


sigset_t get_sigset()
{
// block signals in this thread and subsequently
// spawned threads
sigset_t sigset;
sigemptyset(&sigset);
sigaddset(&sigset, SIGINT);
// sigaddset(&sigset, SIGTERM);
return sigset;
}

#pragma GCC diagnostic pop

std::thread::native_handle_type sig_recv_thread;
std::atomic<bool> receiver_exists(false);
}
Expand Down Expand Up @@ -74,12 +93,7 @@ namespace mamba
{
stop_receiver_thread();

// block signals in this thread and subsequently
// spawned threads
sigset_t sigset;
sigemptyset(&sigset);
sigaddset(&sigset, SIGINT);
// sigaddset(&sigset, SIGTERM);
sigset_t sigset = get_sigset();
pthread_sigmask(SIG_BLOCK, &sigset, nullptr);
std::thread receiver(handler, sigset);
sig_recv_thread = receiver.native_handle();
Expand Down
2 changes: 1 addition & 1 deletion libmamba/src/solver/libsolv/matcher.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ namespace mamba::solver::libsolv
return { std::cref(it->second) };
}

return specs::Channel::resolve(std::move(uc), channel_params())
return specs::Channel::resolve(uc, channel_params())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's const specs::UnresolvedChannel& uc, so no point in std::move(uc)

.transform(
[&](channel_list&& chan)
{
Expand Down
8 changes: 8 additions & 0 deletions libmambapy/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,14 @@ pybind11_add_module(
src/libmambapy/bindings/solver.cpp
src/libmambapy/bindings/solver_libsolv.cpp
)
# TODO: remove when `SubdirData::cache_path()` is removed
if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang")
# This file uses capturing structured bindings, which was fixed in C++20
set_source_files_properties(
src/libmambapy/bindings/legacy.cpp
PROPERTIES COMPILE_FLAGS -Wno-error=deprecated-declarations
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I turned this back to warning. Sometimes it's useful to see when we're using deprecated functions, so let's have it as a warning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense for that code or even the whole library. 👍🏽

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, let's keep it for this file. If needed later, we can add this flag to the whole repo/lib I suppose.
It's better not to use deprecated functions, when possible.

)
endif()

target_include_directories(bindings PRIVATE src/libmambapy/bindings)

Expand Down
Loading