Skip to content

refactor: Fix clang-tidy readability-const-return-type violations#26935

Merged
fanquake merged 1 commit intobitcoin:masterfrom
maflcko:2301-readability-const-return-type-🛥
Feb 1, 2023

Hidden character warning

The head ref may contain hidden characters: "2301-readability-const-return-type-\ud83d\udee5"
Merged

refactor: Fix clang-tidy readability-const-return-type violations#26935
fanquake merged 1 commit intobitcoin:masterfrom
maflcko:2301-readability-const-return-type-🛥

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jan 20, 2023

This comes up during review, so instead of wasting review cycles on this, just enforce it via CI

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 20, 2023 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK stickies-v, 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:

  • #26889 (refactor: wallet, remove global 'ArgsManager' dependency by furszy)
  • #26642 (clang-tidy: Add more performance-* checks and related fixes by hebasto)
  • #26627 (wallet: Migrate non-HD keys with single combo containing a list of keys by achow101)
  • #26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
  • #26152 (Bump unconfirmed ancestor transactions to target feerate by Xekyo)
  • #26008 (wallet: cache IsMine scriptPubKeys to improve performance of wallets with a lot of non-ranged descriptors by achow101)
  • #22693 (RPC/Wallet: Add "use_txids" to output of getaddressinfo by luke-jr)

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.

@hebasto
Copy link
Member

hebasto commented Jan 20, 2023

Concept ACK.

@maflcko maflcko force-pushed the 2301-readability-const-return-type- branch from fa5f55c to faa20bc Compare January 20, 2023 15:38
@maflcko
Copy link
Member Author

maflcko commented Jan 20, 2023

For docs: https://clang.llvm.org/extra/clang-tidy/checks/readability/const-return-type.html

@hebasto
Copy link
Member

hebasto commented Jan 20, 2023

MSVC linker error:

libbitcoin_node.lib(validationinterface.obj) : error LNK2019: unresolved external symbol "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const __cdecl RemovalReasonToString(enum MemPoolRemovalReason const &)" (?RemovalReasonToString@@YA?BV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AEBW4MemPoolRemovalReason@@@Z) referenced in function "public: __cdecl `public: void __cdecl CMainSignals::TransactionRemovedFromMempool(class std::shared_ptr<class CTransaction const > const &,enum MemPoolRemovalReason,unsigned __int64)'::`4'::<lambda_2>::operator()(void)const " (??R<lambda_2>@?3??TransactionRemovedFromMempool@CMainSignals@@QEAAXAEBV?$shared_ptr@$$CBVCTransaction@@@std@@W4MemPoolRemovalReason@@_K@Z@QEBA@XZ) [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\bitcoind\bitcoind.vcxproj]

This was referenced Jan 21, 2023
Merged
Merged
Merged
Closed
@maflcko maflcko force-pushed the 2301-readability-const-return-type- branch from faa20bc to fa7767a Compare January 21, 2023 09:48
@maflcko
Copy link
Member Author

maflcko commented Jan 21, 2023

Thanks, force pushed to remove const from validationinterface.cpp as well.

hebasto
hebasto approved these changes Jan 21, 2023
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK fa7767ae278baa7074735c9eec070d992d2884ff

This was referenced Jan 24, 2023
Merged
Closed
@DrahtBot DrahtBot mentioned this pull request Feb 1, 2023
Merged
@maflcko maflcko force-pushed the 2301-readability-const-return-type- branch from fa7767a to 3333464 Compare February 1, 2023 10:30
@maflcko maflcko force-pushed the 2301-readability-const-return-type- branch from 3333464 to fa451d4 Compare February 1, 2023 10:34
stickies-v
stickies-v approved these changes Feb 1, 2023
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.

utACK fa451d4

I verified that all the removed const qualifiers were top-level, and as such can be safely removed. Checking this with clang-tidy makes the review process more efficient.

hebasto
hebasto approved these changes Feb 1, 2023
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK fa451d4.

Maybe add a commit to address #26705 (comment) as well?

src/test/fuzz/util.h
auto& PickValue(FuzzedDataProvider& fuzzed_data_provider, Collection& col)
{
const auto sz = col.size();
auto sz{col.size()};
Copy link
Member

@hebasto hebasto Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

A note: it looks not related to the readability-const-return-type check, but it does a few lines below in ConsumeIntegralInRange<decltype(sz)>(0, sz - 1).

Copy link
Member Author

@maflcko maflcko Feb 1, 2023

Choose a reason for hiding this comment

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

Correct, decltype(sz) is const ..., which will be the return value of ConsumeIntegralInRange, and thus cause the readability-const-return-type fail

@fanquake fanquake merged commit 550e6bd into bitcoin:master Feb 1, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 1, 2023
@fanquake
Merge bitcoin#26935: refactor: Fix clang-tidy readability-const-retur…
bd39960 
…n-type violations

fa451d4 Fix clang-tidy readability-const-return-type violations (MarcoFalke)

Pull request description:

  This comes up during review, so instead of wasting review cycles on this, just enforce it via CI

ACKs for top commit:
  stickies-v:
    utACK fa451d4
  hebasto:
    ACK fa451d4.

Tree-SHA512: 144a85612f00ec43f7ea1fdaa11901ca981a9f0465a8849745712d741b201b9c3307118172ee0b8efd12bebf25bc6f32a6e2c908495e371f9ada0a917994f44e
@maflcko maflcko deleted the 2301-readability-const-return-type-🛥 branch February 2, 2023 09:53
@bitcoin bitcoin locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

@aureleoules aureleoules Awaiting requested review from aureleoules

+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