Skip to content

ci: add -Wextra-semi to default config - #332

Open
fanquake wants to merge 1 commit into
bitcoin-core:masterfrom
fanquake:drop_redundant_semi
Open

ci: add -Wextra-semi to default config#332
fanquake wants to merge 1 commit into
bitcoin-core:masterfrom
fanquake:drop_redundant_semi

Conversation

@fanquake

@fanquake fanquake commented Aug 4, 2026

Copy link
Copy Markdown
Member

Follow-up to bd50831.

@DrahtBot

DrahtBot commented Aug 4, 2026

Copy link
Copy Markdown

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers
ACK hebasto

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

@maflcko maflcko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems fine, but I am -0

Comment thread test/mp/test/test.cpp Outdated
});
done.get_future().get();
KJ_EXPECT(error_thrown);
KJ_EXPECT(error_thrown)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm, for macros, I generally prefer if they enforce a trailing semicolon.

Though, it may be too late to fix this now?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then perhaps omit -Wextra-semi-stmt?

@fanquake

fanquake commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

Yea. I don't have too much of an opinion, but it wasn't clear to me why a PR labelled "CI" (#321) was also making random source code changes (bd50831) (although it seems like that PR was generally confusing (#321 (comment))), so thought we could at least make this consistent.

@maflcko

maflcko commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

I think 321 was dropping it after a function def, this one is dropping it after statements.

@hebasto

hebasto commented Aug 4, 2026

Copy link
Copy Markdown
Member

... random source code changes (bd50831)...

See #321 (comment).

@fanquake

fanquake commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

See #321 (comment).

Thanks. It would be good to write commit messages that contain information for other contributors (which then persists over time), so they don't have to search through GitHub comments to try and find context/explanations.

@fanquake
fanquake force-pushed the drop_redundant_semi branch from bf3dbbc to 49834b2 Compare August 6, 2026 11:17
@fanquake fanquake changed the title refactor: drop redundant semicolons ci: add -Wextra-semi to default config Aug 6, 2026
@fanquake

fanquake commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

Changed to just add -Wextra-semi, to prevent some re-introduction.

@hebasto hebasto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK 49834b2.

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.

4 participants