Fix clang-tidy warnings in recently introduced code#3323
Conversation
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #3323 +/- ##
=======================================
Coverage 88.09% 88.10%
=======================================
Files 615 615
Lines 69634 69628 -6
Branches 6908 6905 -3
=======================================
+ Hits 61342 61343 +1
+ Misses 5412 5400 -12
- Partials 2880 2885 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
reneme
left a comment
There was a problem hiding this comment.
LGTM. Is there something that stops us from running clang-tidy in CI to catch those issues early?
| const auto& [_, session_and_handle] = item; | ||
| const auto& [__, this_handle] = session_and_handle; | ||
|
|
||
| const auto& [_unused, this_handle] = session_and_handle; |
There was a problem hiding this comment.
Maybe _unused1 and _unused2 for consistency? I found double-underscore was kind-of good enough to work around the 'repeated identifier' issue. But if clang-tidy doesn't like that, so be it.
There was a problem hiding this comment.
Yeah that's fine. For context the complaint here is because C++ reserves all identifiers containing __ for the implementation.
|
The main reason to not run it in CI is runtime. It takes about 15 minutes to run Maybe we can hack up something that only runs it over source files that changed in a PR - that will still miss some problems but would catch the majority before merge, which would be an improvement over the current situation. |
cc @reneme on the TLS session manager changes