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

chore: Fix all clang build warnings #4475

Merged
merged 4 commits into from
Jan 20, 2025
Merged

chore: Fix all clang build warnings #4475

merged 4 commits into from
Jan 20, 2025

Conversation

chakaz
Copy link
Collaborator

@chakaz chakaz commented Jan 19, 2025

Also add -Werror to clang build in CI.

Fixes #4449

@chakaz chakaz changed the title [DO NOT REVIEW YET] chore: Fix all clang build warnings chore: Fix all clang build warnings Jan 19, 2025
@chakaz chakaz requested a review from adiholden January 19, 2025 09:19
@adiholden
Copy link
Collaborator

@chakaz what are the changes in helio?

@chakaz
Copy link
Collaborator Author

chakaz commented Jan 19, 2025

@chakaz what are the changes in helio?

romange/helio#372

Without which we can't enable -Werror with clang

@adiholden
Copy link
Collaborator

@chakaz what are the changes in helio?

romange/helio#372

Without which we can't enable -Werror with clang

Ok. The PR changes LGTM. Lets only get @romange approval for pulling latest helio

@@ -1807,7 +1807,7 @@ void DestroyGroup(facade::CmdArgParser* parser, Transaction* tx, SinkReplyBuilde
if (parser->HasNext())
return builder->SendError(UnknownSubCmd("DESTROY", "XGROUP"));

auto cb = [&](Transaction* t, EngineShard* shard) {
auto cb = [&, &key = key, &gname = gname](Transaction* t, EngineShard* shard) {
Copy link
Contributor

@kostasrim kostasrim Jan 20, 2025

Choose a reason for hiding this comment

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

pfff captured structure bindings requiring c++20 extension to compile without warning pre C++20 😱

@chakaz
Copy link
Collaborator Author

chakaz commented Jan 20, 2025

@adiholden we have an OK from @romange 🥳
Please approve

@kostasrim
Copy link
Contributor

@adiholden we have an OK from @romange 🥳 Please approve

she is away, I accepted it 😄

@chakaz chakaz merged commit 6f3c6e3 into main Jan 20, 2025
9 checks passed
@chakaz chakaz deleted the chakaz/clang-clean branch January 20, 2025 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eliminate clang warnings
3 participants