Skip to content

clang-tidy: fix warnings introduced in version 19 #165

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

clang-tidy: fix warnings introduced in version 19 #165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
764128a
disable linting of generated files
purpleKarrot Feb 28, 2025
8ed7b63
replace SFINAE trick with `if constexpr`
purpleKarrot Feb 28, 2025
36898db
replace custom tuple unpacking code with `std::apply`
purpleKarrot Feb 28, 2025
9a01a35
make member function const
purpleKarrot Feb 28, 2025
049d931
use ranges transform
purpleKarrot Feb 28, 2025
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
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ configure_file(include/mp/config.h.in "${CMAKE_CURRENT_BINARY_DIR}/include/mp/co

# Generated C++ Capn'Proto schema files
capnp_generate_cpp(MP_PROXY_SRCS MP_PROXY_HDRS include/mp/proxy.capnp)
set_source_files_properties(${MP_PROXY_SRCS} PROPERTIES SKIP_LINTING ON)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In commit "disable linting of generated files" (764128a)

Seems ok to skip linting, but in general I think it would be better if we could run the linters on generated files to be alerted about problems in generated code that we could fix. Last time I tried running linters there were just a few modernize-use-override errors which make sense to disable because generated code can't know if classes it is inheriting from use virtual methods or not. The fix I made at the time was:

diff 

--- a/src/ipc/libmultiprocess/src/mp/gen.cpp
+++ b/src/ipc/libmultiprocess/src/mp/gen.cpp
@@ -252,6 +256,7 @@ static void Generate(kj::StringPtr src_prefix,
     h << "#pragma GCC diagnostic ignored \"-Wsuggest-override\"\n";
     h << "#endif\n";
     h << "#endif\n";
+    h << "// NOLINTBEGIN(modernize-use-override)\n";
     h << "namespace mp {\n";
 
     kj::StringPtr message_namespace;
@@ -628,6 +633,7 @@ static void Generate(kj::StringPtr src_prefix,
     inl << "#endif\n";
 
     h << "} // namespace mp\n";
+    h << "// NOLINTEND(modernize-use-override)\n";
     h << "#if defined(__GNUC__)\n";
     h << "#pragma GCC diagnostic pop\n";
     h << "#endif\n";

And I would still recommend that change as an alternative to this commit. Current commit also seems ok though and I always can PR the other change separately.


# util library
add_library(mputil OBJECT src/mp/util.cpp)
Expand Down
4 changes: 3 additions & 1 deletion cmake/TargetCapnpSources.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,14 @@ function(target_capnp_sources target include_prefix)
DEPENDS ${capnp_file}
VERBATIM
)
target_sources(${target} PRIVATE
set(generated_sources
${CMAKE_CURRENT_BINARY_DIR}/${capnp_file}.c++
${CMAKE_CURRENT_BINARY_DIR}/${capnp_file}.proxy-client.c++
${CMAKE_CURRENT_BINARY_DIR}/${capnp_file}.proxy-server.c++
${CMAKE_CURRENT_BINARY_DIR}/${capnp_file}.proxy-types.c++
)
set_source_files_properties(${generated_sources} PROPERTIES SKIP_LINTING ON)
target_sources(${target} PRIVATE ${generated_sources})

list(APPEND generated_headers ${capnp_file}.h)
endforeach()
Expand Down
2 changes: 1 addition & 1 deletion include/mp/proxy-io.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ class EventLoop
void addClient(std::unique_lock<std::mutex>& lock);
bool removeClient(std::unique_lock<std::mutex>& lock);
//! Check if loop should exit.
bool done(std::unique_lock<std::mutex>& lock);
bool done(std::unique_lock<std::mutex>& lock) const;

Logger log()
{
Expand Down
128 changes: 63 additions & 65 deletions include/mp/proxy-types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,45 @@ struct StructField
}
Struct& m_struct;

// clang-format off
template<typename A = Accessor> auto get() const -> decltype(A::get(this->m_struct)) { return A::get(this->m_struct); }
template<typename A = Accessor> auto has() const -> std::enable_if_t<A::optional, bool> { return A::getHas(m_struct); }
template<typename A = Accessor> auto has() const -> std::enable_if_t<!A::optional && A::boxed, bool> { return A::has(m_struct); }
template<typename A = Accessor> auto has() const -> std::enable_if_t<!A::optional && !A::boxed, bool> { return true; }
template<typename A = Accessor> auto want() const -> std::enable_if_t<A::requested, bool> { return A::getWant(m_struct); }
template<typename A = Accessor> auto want() const -> std::enable_if_t<!A::requested, bool> { return true; }
template<typename A = Accessor, typename... Args> decltype(auto) set(Args&&... args) const { return A::set(this->m_struct, std::forward<Args>(args)...); }
template<typename A = Accessor, typename... Args> decltype(auto) init(Args&&... args) const { return A::init(this->m_struct, std::forward<Args>(args)...); }
template<typename A = Accessor> auto setHas() const -> std::enable_if_t<A::optional> { return A::setHas(m_struct); }
template<typename A = Accessor> auto setHas() const -> std::enable_if_t<!A::optional> { }
template<typename A = Accessor> auto setWant() const -> std::enable_if_t<A::requested> { return A::setWant(m_struct); }
template<typename A = Accessor> auto setWant() const -> std::enable_if_t<!A::requested> { }
// clang-format on
decltype(auto) get() const { return Accessor::get(this->m_struct); }

bool has() const {
if constexpr (Accessor::optional) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In commit "replace SFINAE trick with if constexpr" (8ed7b63)

Seems like good changes, but is indentation in this part of the code supposed to use 4 spaces instead of 2? Would be good to make this consistent and maybe use clang-format

return Accessor::getHas(m_struct);
} else if constexpr (Accessor::boxed) {
return Accessor::has(m_struct);
} else {
return true;
}
}

bool want() const {
if constexpr (Accessor::requested) {
return Accessor::getWant(m_struct);
} else {
return true;
}
}

template <typename... Args> decltype(auto) set(Args &&...args) const {
return Accessor::set(this->m_struct, std::forward<Args>(args)...);
}

template <typename... Args> decltype(auto) init(Args &&...args) const {
return Accessor::init(this->m_struct, std::forward<Args>(args)...);
}

void setHas() const {
if constexpr (Accessor::optional) {
Accessor::setHas(m_struct);
}
}

void setWant() const {
if constexpr (Accessor::requested) {
Accessor::setWant(m_struct);
}
}
};


Expand Down Expand Up @@ -364,30 +389,17 @@ struct ClientParam

struct BuildParams : IterateFieldsHelper<BuildParams, sizeof...(Types)>
{
template <typename... Args>
void handleField(Args&&... args)
{
callBuild<0>(std::forward<Args>(args)...);
}

// TODO Possible optimization to speed up compile time:
// https://stackoverflow.com/a/7858971 Using enable_if below to check
// position when unpacking tuple might be slower than pattern matching
// approach in the stack overflow solution
template <size_t I, typename... Args>
auto callBuild(Args&&... args) -> std::enable_if_t<(I < sizeof...(Types))>
{
callBuild<I + 1>(std::forward<Args>(args)..., std::get<I>(m_client_param->m_values));
}

template <size_t I, typename Params, typename ParamList, typename... Values>
auto callBuild(ClientInvokeContext& invoke_context, Params& params, ParamList, Values&&... values) ->
std::enable_if_t<(I == sizeof...(Types))>
template <typename Params, typename ParamList>
void handleField(ClientInvokeContext& invoke_context, Params& params, ParamList)
{
MaybeBuildField(std::integral_constant<bool, Accessor::in>(), ParamList(), invoke_context,
Make<StructField, Accessor>(params), std::forward<Values>(values)...);
MaybeSetWant(
ParamList(), Priority<1>(), std::forward<Values>(values)..., Make<StructField, Accessor>(params));
auto const fun = [&]<typename... Values>(Values&&... values) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In commit "replace custom tuple unpacking code with std::apply" (36898db)

I think this code might be more straightforward and easier to review if this got rid of the fun variables and just passed the lambda directly as the first argument to std::apply. That way the indentation of the code inside the lambda would not have to change and the diff would be smaller and more obvious, and the resulting code would also be shorter.

MaybeBuildField(std::integral_constant<bool, Accessor::in>(), ParamList(), invoke_context,
Make<StructField, Accessor>(params), std::forward<Values>(values)...);
MaybeSetWant(
ParamList(), Priority<1>(), std::forward<Values>(values)..., Make<StructField, Accessor>(params));
};

std::apply(fun, m_client_param->m_values);
}

BuildParams(ClientParam* client_param) : m_client_param(client_param) {}
Expand All @@ -396,24 +408,15 @@ struct ClientParam

struct ReadResults : IterateFieldsHelper<ReadResults, sizeof...(Types)>
{
template <typename... Args>
void handleField(Args&&... args)
template <typename Results, typename... Params>
void handleField(ClientInvokeContext& invoke_context, Results& results, TypeList<Params...>)
{
callRead<0>(std::forward<Args>(args)...);
}
auto const fun = [&]<typename... Values>(Values&&... values) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In commit "replace custom tuple unpacking code with std::apply" (36898db)

Note: Values type here is currently unused, so lambda declaration could be simplified to just take an auto&& values parameter. However, keeping Values is nice even though it is unused because it makes ReadParams and BuildResults code more consistent. Also it might be useful to add perfect forwarding for ReadDestUpdate(std::forward<Values>(values)) in the future. That will require other changes to this code however, so better to leave alone for now.

In general there are a lot more simplifications that can be made here now that this code no longer needs to work with c++11. Would be good to follow up in a separate PR.

MaybeReadField(std::integral_constant<bool, Accessor::out>(), TypeList<Decay<Params>...>(), invoke_context,
Make<StructField, Accessor>(results), ReadDestUpdate(values)...);
};

template <int I, typename... Args>
auto callRead(Args&&... args) -> std::enable_if_t<(I < sizeof...(Types))>
{
callRead<I + 1>(std::forward<Args>(args)..., std::get<I>(m_client_param->m_values));
}

template <int I, typename Results, typename... Params, typename... Values>
auto callRead(ClientInvokeContext& invoke_context, Results& results, TypeList<Params...>, Values&&... values)
-> std::enable_if_t<I == sizeof...(Types)>
{
MaybeReadField(std::integral_constant<bool, Accessor::out>(), TypeList<Decay<Params>...>(), invoke_context,
Make<StructField, Accessor>(results), ReadDestUpdate(values)...);
std::apply(fun, m_client_param->m_values);
}

ReadResults(ClientParam* client_param) : m_client_param(client_param) {}
Expand Down Expand Up @@ -650,19 +653,14 @@ void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, Fiel
//! return value with value of `ret()`. This is useful for avoiding code
//! duplication and branching in generic code that forwards calls to functions.
template <typename Fn, typename Ret>
auto ReplaceVoid(Fn&& fn, Ret&& ret) ->
std::enable_if_t<std::is_same_v<void, decltype(fn())>, decltype(ret())>
auto ReplaceVoid(Fn&& fn, Ret&& ret)
{
fn();
return ret();
}

//! Overload of above for non-void `fn()` case.
template <typename Fn, typename Ret>
auto ReplaceVoid(Fn&& fn, Ret&& ret) ->
std::enable_if_t<!std::is_same_v<void, decltype(fn())>, decltype(fn())>
{
return fn();
if constexpr (std::is_same_v<decltype(fn()), void>) {
fn();
return ret();
} else {
return fn();
}
}

extern std::atomic<int> server_reqs;
Expand Down
2 changes: 1 addition & 1 deletion src/mp/gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ static void Generate(kj::StringPtr src_prefix,
cpp_types << "namespace mp {\n";

std::string guard = output_path;
std::transform(guard.begin(), guard.end(), guard.begin(), [](unsigned char c) -> unsigned char {
std::ranges::transform(guard, guard.begin(), [](unsigned char c) -> unsigned char {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In commit "use ranges transform" (049d931)

It's not really clear why this new code is better here. Would be good to have some explanation or note what the warning is in the commit message.

if ('0' <= c && c <= '9') return c;
if ('A' <= c && c <= 'Z') return c;
if ('a' <= c && c <= 'z') return c - 'a' + 'A';
Expand Down
2 changes: 1 addition & 1 deletion src/mp/proxy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ void EventLoop::startAsyncThread(std::unique_lock<std::mutex>& lock)
}
}

bool EventLoop::done(std::unique_lock<std::mutex>& lock)
bool EventLoop::done(std::unique_lock<std::mutex>& lock) const
{
assert(m_num_clients >= 0);
assert(lock.owns_lock());
Expand Down