refactor: modernize-use-equals-default #30406

1. 
   maflcko commented at 9:20 am on July 8, 2024: member

   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

2. tidy: modernize-use-equals-default 3333bae9b2
3. 
   DrahtBot commented at 9:20 am on July 8, 2024: contributor

   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.

4. maflcko renamed this:
   tidy: modernize-use-equals-default
   refactor: modernize-use-equals-default
   on Jul 8, 2024
5. DrahtBot added the label Refactoring on Jul 8, 2024
6. 
   hebasto commented at 1:35 pm on July 10, 2024: member

   Concept ACK.
7. stickies-v approved
8. 
   stickies-v commented at 11:29 am on July 11, 2024: contributor

   ACK 3333bae9b2a6c1ee2314d33361c93944c12001f9

   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.

9. DrahtBot requested review from hebasto on Jul 11, 2024
10. 
    maflcko commented at 11:45 am on July 11, 2024: member

    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 (review)

    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.

11. fanquake merged this on Jul 11, 2024
12. fanquake closed this on Jul 11, 2024

    ---

13. maflcko deleted the branch on Jul 12, 2024

Contributors
maflcko DrahtBot hebasto stickies-v

Review Requested
hebasto

Labels
Refactoring