Repository navigation
Satisfy clang-tidy-21 - #171
christophfroehlich wants to merge 15 commits into
Conversation
3fcdaba to
8c77e4c
Compare
| #ifndef RSL_ALGORITHM_HPP_ | ||
| #define RSL_ALGORITHM_HPP_ |
There was a problem hiding this comment.
Why are you replacing pragma once with define guards? Is this still a problem for someone in 2026?
There was a problem hiding this comment.
There’s a new-ish clang-tidy check in the portability-* family for removing #pragma once. We obviously don’t want to use that check.
There was a problem hiding this comment.
I only use this for ROS packages and there the define include guards are de facto standard afaik? That's the only reason, I don't care at all, I just wanted to satisfy all the checks.
There was a problem hiding this comment.
I would not say either is a de facto standard, although I do think #pragma once has been the more popular choice in the greater C++ community for years. It’s a lot nicer to read and write and lacks some of the common pitfalls of header guards, namely how you can accidentally give two headers the same guard name.
I just wanted to satisfy all the checks.
Many clang-tidy checks exist to enforce particular opinions. It’s a misconception that all clang-tidy checks will improve your code. We should only enable checks that improve our code or codify existing opinions we hold. I don’t think any of the new checks that got implicitly enabled are worth fixing. We should amend the .clang-tidy file to disable them.
|
|
||
| #include <tl/expected.hpp> | ||
|
|
||
| #if defined(__cpp_concepts) && (__cpp_concepts >= 201907L) |
There was a problem hiding this comment.
I’d much rather just upgrade the whole library to C++20 than conditionally use it. I don’t like this half measure.
| */ | ||
| [[nodiscard]] auto size() const noexcept { | ||
| auto const lock = std::lock_guard(mutex_); | ||
| auto const lock = std::scoped_lock(mutex_); |
There was a problem hiding this comment.
This is also a clang-tidy warning I have disabled recently. I don’t think scoped_lock is an improvement. It’s not worse either.
| namespace { | ||
|
|
||
| #if defined(__cpp_lib_math_constants) && (__cpp_lib_math_constants >= 201907L) | ||
| constexpr auto k_pi = std::numbers::pi_v<double>; |
There was a problem hiding this comment.
Either upgrade to C++20 or not. Let’s not try to flirt between both standards. This is not making the code any better.
| for (auto& consumer : consumers) consumer.join(); | ||
|
|
||
| CHECK(queue.size() == thread_count * item_count - items_removed); | ||
| CHECK(queue.size() == (thread_count * item_count) - items_removed); |
There was a problem hiding this comment.
The clang-tidy check about adding parens for readability’s sake isn’t one I’m a fan of either. C++ operator precedence is well-defined. I’m fine depending on it.
|
I don’t think any of these checks are worth addressing. Some are totally irrelevant, notably anything involving C++20 since to my knowledge this library still uses C++17. However, it should upgrade to C++20. There’s no good reason to avoid C++20 anymore these days (as long as you don’t touch named modules or header units). |
3 participants
No description provided.