Skip to content

refactor: modernize-use-equals-default #30406

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

Jump to bottom
Merged
merged 1 commit into from
Jul 11, 2024
Merged

refactor: modernize-use-equals-default #30406

merged 1 commit into from
Jul 11, 2024

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jul 8, 2024 •
edited
Loading

Prior to C++20, modernize-use-equals-default could have been problematic because it could turn a non-aggregate into an aggregate. The risk would be that aggregate initialization would be enabled where the author did not intend to enable it.

With C++20, aggregate for those is forbidden either way. (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1008r1.pdf)

So enabled it for code clarity, consistency, and possibly unlocking compiler optimizations. See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 8, 2024 •
edited
Loading

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK stickies-v
Concept ACK hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
  • #30341 (WIP: Permit Combiner to strip bip32_deriv information by willcl-ark)
  • #29369 (refactor: Allow CScript construction from any std::input_iterator by maflcko)
  • #29280 (Implement OP_CHECKTEMPLATEVERIFY by reardencode)
  • #21283 (Implement BIP 370 PSBTv2 by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@maflcko maflcko changed the title tidy: modernize-use-equals-default refactor: modernize-use-equals-default Jul 8, 2024
@maflcko maflcko mentioned this pull request Jul 8, 2024
Merged
This was referenced Jul 8, 2024
Draft
Draft
Merged
Closed
Open
@hebasto
Copy link
Member

hebasto commented Jul 10, 2024

Concept ACK.

stickies-v
stickies-v approved these changes Jul 11, 2024
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

ACK 3333bae

My understanding is that prior to C++20, aggregates could not have user-provided constructors and destructors, but could have user-declared ones. Since C++20, the requirements to be an aggregate have been tightened to not allow user-declared ctors and dtors either. As such, this proposed change does not affect initialization of the affected structs and classes.

I would think the benefits are mild, but I don't see any real downsides either - easy to review, and minimal (easy-to-resolve) conflicts. Added friction for developers seems minimal too, and issues are trivially resolved after CI tidy failure.

@DrahtBot DrahtBot requested a review from hebasto July 11, 2024 11:29
@maflcko
Copy link
Member Author

maflcko commented Jul 11, 2024

I would think the benefits are mild, but I don't see any real downsides either - easy to review, and minimal (easy-to-resolve) conflicts. Added friction for developers seems minimal too, and issues are trivially resolved after CI tidy failure.

Agree that the performance benefits are mild at best, if any at all. At this point it is mostly for consistency, because most of the codebase already uses =default. Also, it makes meta-programming easier, as well as writing new tidy checks, knowing that an empty body doesn't exist. See #30146 (comment)

All of the listed conflicts are either drafts or unreviewed, so it seems fine.

Though, happy to close, if others think the benefits are not worth it.

@fanquake fanquake merged commit 01dc38b into bitcoin:master Jul 11, 2024
@maflcko maflcko deleted the 2407-modernize-use-equals-default branch July 12, 2024 09:17
@ryanofsky ryanofsky mentioned this pull request Jul 19, 2024
Open
@bitcoin bitcoin locked and limited conversation to collaborators Jul 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto Awaiting requested review from hebasto

1 more reviewer

@stickies-v stickies-v stickies-v approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maflcko @DrahtBot @hebasto @stickies-v @fanquake