Annotate unused parameters with [[maybe_unused]] #14194

pull practicalswift wants to merge 3 commits into bitcoin:master from practicalswift:annotate-intentionally-unused-parameters changing 8 files +40 −20

practicalswift commented at 9:22 PM on September 10, 2018: contributor

Annotate unused parameters with [[maybe_unused]].
practicalswift force-pushed on Sep 10, 2018
fanquake added the label Build system on Sep 10, 2018
DrahtBot commented at 11:50 PM on September 10, 2018: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #14605 (Return of the Banman by dongcarl)
- #14224 (Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations by practicalswift)
- #13864 (validation: Document where we are intentionally ignoring bool return values from validation related functions by practicalswift)
- #13815 (util: Add [[nodiscard]] to all {Decode,Parse}... functions returning bool by practicalswift)
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 cross-referenced this on Sep 11, 2018 from issue Add tests and error handling to DecodeExtPubKey/DecodeExtKey. Add [[nodiscard]]. by practicalswift
DrahtBot cross-referenced this on Sep 11, 2018 from issue validation: Document where we are intentionally ignoring bool return values from validation related functions by practicalswift
DrahtBot cross-referenced this on Sep 11, 2018 from issue util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool by practicalswift
Empact commented at 5:09 AM on September 11, 2018: member

Concept ACK maybe_unused is a C++17 introduction, I assume it's ok to use it though based on preprocessor activation.
practicalswift commented at 7:55 AM on September 11, 2018: contributor

@Empact Yes, it is worth noting that both clang++ and g++ support [[maybe_unused]] also under -std=c++11. Luckily no need for wait for C++17 :-)
Empact commented at 12:10 AM on September 12, 2018: member

BlockNotifyGenesisWait uses unnamed args to mark them as unreferenced. Should we name there or apply that here?

in src/attributes.h:11 in a363c5d1e6 outdated

   6 | +#ifndef BITCOIN_ATTRIBUTES_H
   7 | +#define BITCOIN_ATTRIBUTES_H
   8 | +
   9 | +#if defined(__has_cpp_attribute) && __has_cpp_attribute(maybe_unused)
  10 | +#  define MAYBE_UNUSED [[maybe_unused]]
  11 | +#elif defined(__GNUC__) || defined(__clang__)

ken2812221 commented at 3:26 AM on September 12, 2018:

__GNUC__ is also defined by clang. Don't have to check __clang__

Introduce MAYBE_UNUSED ([maybe_unused]]) b7ca9568c3
practicalswift force-pushed on Sep 12, 2018
practicalswift commented at 3:13 PM on September 12, 2018: contributor

@ken2812221 Updated to address feedback. Please re-review :-)
DrahtBot cross-referenced this on Sep 15, 2018 from issue Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations by practicalswift

in src/rpc/blockchain.h:29 in 54b3d00e24 outdated

  25 | @@ -24,7 +26,7 @@ static constexpr int NUM_GETBLOCKSTATS_PERCENTILES = 5;
  26 |  double GetDifficulty(const CBlockIndex* blockindex);
  27 |  
  28 |  /** Callback for when block tip changed. */
  29 | -void RPCNotifyBlockChange(bool ibd, const CBlockIndex *);
  30 | +void RPCNotifyBlockChange(MAYBE_UNUSED bool ibd, const CBlockIndex *);

arvidn commented at 2:11 PM on September 17, 2018:

it seems odd that this would be necessary in a declaration. instead of adding the attribute here, you could just remove the name of the parameter (just like for the second parameter). neither are used in the declaration anyway.

in src/rpc/blockchain.cpp:194 in 54b3d00e24 outdated

 190 | @@ -191,7 +191,7 @@ static UniValue getbestblockhash(const JSONRPCRequest& request)
 191 |      return chainActive.Tip()->GetBlockHash().GetHex();
 192 |  }
 193 |  
 194 | -void RPCNotifyBlockChange(bool ibd, const CBlockIndex * pindex)
 195 | +void RPCNotifyBlockChange(MAYBE_UNUSED bool ibd, const CBlockIndex * pindex)

arvidn commented at 2:12 PM on September 17, 2018:

ibd seems to always be unused here. I think a better approach is to just remove the argument name

in src/qt/transactiontablemodel.cpp:725 in 54b3d00e24 outdated

 721 | @@ -722,7 +722,7 @@ static void NotifyTransactionChanged(TransactionTableModel *ttm, const uint256 &
 722 |      notification.invoke(ttm);
 723 |  }
 724 |  
 725 | -static void ShowProgress(TransactionTableModel *ttm, const std::string &title, int nProgress)
 726 | +static void ShowProgress(TransactionTableModel *ttm, MAYBE_UNUSED const std::string &title, int nProgress)

arvidn commented at 2:13 PM on September 17, 2018:

title appears to be unused as well. I think a better approach is to remove the parameter name.

DrahtBot cross-referenced this on Sep 20, 2018 from issue Skip tx-rehashing on historic blocks by MarcoFalke
Annotate unused parameters with [[maybe_unused]] 9cd951cc77
practicalswift force-pushed on Sep 21, 2018
doc: Add a developer note about MAYBE_UNUSED 8d63c12279
practicalswift commented at 2:51 PM on September 21, 2018: contributor

@arvidn Good points. Now updated. Also added a developer note. Please re-review :-)
DrahtBot commented at 11:41 AM on September 28, 2018: contributor



Coverage Change (pull 14194) Reference (master)

Lines +0.0375 % 87.0361 %

Functions +0.1235 % 84.1130 %

Branches +0.0095 % 51.5451 %
practicalswift cross-referenced this on Oct 22, 2018 from issue Enable flake8 rule E225 which checks for missing whitespace around op… by jbampton
DrahtBot cross-referenced this on Oct 30, 2018 from issue Return of the Banman by dongcarl
practicalswift closed this on Nov 12, 2018
practicalswift deleted the branch on Apr 10, 2021
bitcoin locked this on Aug 16, 2022

Coverage	Change (pull 14194)	Reference (master)
Lines	+0.0375 %	87.0361 %
Functions	+0.1235 %	84.1130 %
Branches	+0.0095 %	51.5451 %

Contributors

Labels

Linked (view graph)

#14534 Enable flake8 rule E225 which checks for missing whitespace around op…

Annotate unused parameters with [[maybe_unused]] #14194

Conflicts