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

maflcko commented at 3:06 PM on January 20, 2023: member

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

DrahtBot commented at 3:06 PM on January 20, 2023: contributor

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.

DrahtBot added the label Refactoring on Jan 20, 2023

hebasto commented at 3:23 PM on January 20, 2023: member

Concept ACK.

maflcko force-pushed on Jan 20, 2023

maflcko commented at 4:10 PM on January 20, 2023: member

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

hebasto commented at 9:05 PM on January 20, 2023: member

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]

DrahtBot cross-referenced this on Jan 21, 2023 from issue refactor: wallet, remove global 'ArgsManager' dependency by furszy

DrahtBot cross-referenced this on Jan 21, 2023 from issue clang-tidy: Add more `performance-*` checks and related fixes by hebasto

DrahtBot cross-referenced this on Jan 21, 2023 from issue wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101

DrahtBot cross-referenced this on Jan 21, 2023 from issue RPC/Wallet: Add "use_txids" to output of getaddressinfo by luke-jr

maflcko force-pushed on Jan 21, 2023

maflcko commented at 11:38 AM on January 21, 2023: member

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

hebasto approved

hebasto commented at 12:51 PM on January 21, 2023: member

ACK fa7767ae278baa7074735c9eec070d992d2884ff

DrahtBot cross-referenced this on Jan 24, 2023 from issue wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101

DrahtBot cross-referenced this on Jan 26, 2023 from issue wallet: Migrate non-HD keys with single combo containing a list of keys by achow101

DrahtBot cross-referenced this on Feb 1, 2023 from issue Bump unconfirmed ancestor transactions to target feerate by murchandamus

maflcko force-pushed on Feb 1, 2023

Fix clang-tidy readability-const-return-type violations fa451d4b60

maflcko force-pushed on Feb 1, 2023

fanquake requested review from aureleoules on Feb 1, 2023

fanquake requested review from stickies-v on Feb 1, 2023

fanquake requested review from hebasto on Feb 1, 2023

stickies-v approved

stickies-v commented at 12:36 PM on February 1, 2023: contributor

utACK fa451d4b6

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.

in src/test/fuzz/util.h:50 in fa451d4b60

  46 | @@ -47,7 +47,7 @@ size_t CallOneOf(FuzzedDataProvider& fuzzed_data_provider, Callables... callable
  47 |  template <typename Collection>
  48 |  auto& PickValue(FuzzedDataProvider& fuzzed_data_provider, Collection& col)
  49 |  {
  50 | -    const auto sz = col.size();
  51 | +    auto sz{col.size()};

hebasto commented at 12:54 PM on February 1, 2023:

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).

maflcko commented at 1:16 PM on February 1, 2023:

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

hebasto approved

hebasto commented at 1:03 PM on February 1, 2023: member

ACK fa451d4b60ee0538b3ea6b946740a64734b35b6d.

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

fanquake merged this on Feb 1, 2023

fanquake closed this on Feb 1, 2023

sidhujag referenced this in commit bd399601bd on Feb 1, 2023

maflcko deleted the branch on Feb 2, 2023

bitcoin locked this on Feb 2, 2024