util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool #13815

pull practicalswift wants to merge 4 commits into bitcoin:master from practicalswift:check-return-values-from-parse changing 15 files +84 −53
  1. practicalswift commented at 5:50 AM on July 31, 2018: contributor

    Changes in this PR:

    • Add linter to make sure the return value of Parse[...](...) is checked
    • Add __attribute__((warn_unused_result)) to all {Decode,Parse}[...](...) functions returning bool
    • Fix violations

    Context:

    • #13712: wallet: Fix non-determinism in ParseHDKeypath(...). Avoid using an uninitialized variable in path calculation. would have been prevented by this
  2. fanquake added the label Scripts and tools on Jul 31, 2018
  3. fanquake commented at 5:52 AM on July 31, 2018: member 
    $ test/lint/lint-all.sh
Missing "export LC_ALL=C" (to avoid locale dependence) as first non-comment non-empty line in test/lint/lint-cpp.sh
^---- failure generated from test/lint/lint-shell-locale.sh
The command "test/lint/lint-all.sh" exited with 1.
  4. in src/miner.cpp:79 in ee225c8913 outdated 
      76 | -        options.blockMinFeeRate = CFeeRate(n);
  77 | +        if (ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) {
  78 | +            options.blockMinFeeRate = CFeeRate(n);
  79 | +        } else {
  80 | +            options.blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE);
  81 | +        }
    Empact commented at 7:03 AM on July 31, 2018:

    You could avoid the duplicate else clause by boosting amount out like so:

    if (gArgs.IsArgSet("-blockmintxfee") && ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) {
  5. practicalswift force-pushed on Jul 31, 2018
  6. practicalswift commented at 7:52 AM on July 31, 2018: contributor

    @fanquake Thanks! lint-shell-locale.sh to the rescue once again! Now fixed! Please re-review :-) @Empact Good point! Updated. Please re-review :-)

  7. skeees commented at 3:38 AM on August 2, 2018: contributor

    utACK (weakly regarding the linter) If you want to have lots of lint checks and similar static analysis to the kind you are adding here (it seems you do, and these things are useful!) - it might make sense to consider investing in building out tooling that uses libclang and inspects the AST directly as opposed to just regexp matching.

  8. practicalswift renamed this:
    build: Add linter to make sure the return value of Parse[...](...) is checked
    Add __attribute__((warn_unused_result)) to all Parse[...](...) functions
    on Aug 2, 2018
  9. practicalswift force-pushed on Aug 2, 2018
  10. practicalswift commented at 1:48 PM on August 2, 2018: contributor

    @skeees I removed the linter. Now using __attribute__((warn_unused_result)) instead. Please re-review :-)

  11. practicalswift force-pushed on Aug 2, 2018
  12. skeees commented at 2:20 PM on August 2, 2018: contributor

    :) Nice Is is best practice to #define this attribute similar to how this is done in threadsafety.h or # define SECP256K1_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__)) ?

  13. practicalswift force-pushed on Aug 2, 2018
  14. practicalswift force-pushed on Aug 2, 2018
  15. practicalswift commented at 2:38 PM on August 2, 2018: contributor

    @skeees The secp256k1 define is used to handle ancient gcc versions AFAICS, so I see no reason to use that here. Let me know if you see any other reason :-)

  16. practicalswift force-pushed on Aug 2, 2018
  17. practicalswift force-pushed on Aug 2, 2018
  18. practicalswift force-pushed on Aug 2, 2018
  19. practicalswift renamed this:
    Add __attribute__((warn_unused_result)) to all Parse[...](...) functions
    Add __attribute__((warn_unused_result)) to all {Decode,Parse}[...](...) functions returning bool
    on Aug 2, 2018
  20. practicalswift commented at 2:56 PM on August 2, 2018: contributor

    Also added __attribute__((warn_unused_result)) to all Decode... functions returning bool. Please re-review :-)

  21. DrahtBot commented at 3:09 PM on August 2, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #14605 (Return of the Banman by dongcarl)
    • #14045 (WIP: refactor: Fix the chainparamsbase -> util circular dependency by Empact)

    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.

  22. skeees commented at 3:33 PM on August 2, 2018: contributor

    can reorder the Tests* commits first before adding the attribute commits so that you don't break a bisect

  23. practicalswift commented at 3:44 PM on August 2, 2018: contributor

    @skeees When rethinking it I think you have a good point regarding using a #define. Perhaps we should go with:

    #ifdef __has_cpp_attribute
#  if __has_cpp_attribute(nodiscard)
#    define NODISCARD [[nodiscard]]
#  endif
#else
#  define NODISCARD __attribute__((warn_unused_result))
#endif

    That will have the benefit of being less verbose. That would use the newer [[nodiscard]] (required from C++17) where available and fall back on the older __attribute__((warn_unused_result)).

    Makes sense?

    What would be the most appropriate file to put that #define in? @MarcoFalke, any input?

  24. practicalswift force-pushed on Aug 2, 2018
  25. practicalswift force-pushed on Aug 2, 2018
  26. practicalswift commented at 3:49 PM on August 2, 2018: contributor

    @skeees Commits reordered as requested!

  27. skeees commented at 3:51 PM on August 2, 2018: contributor

    I like that #define solution @practicalswift I don't see an obviously appropriate place for these right now maybe add staticanalysis.h ?

  28. practicalswift renamed this:
    Add __attribute__((warn_unused_result)) to all {Decode,Parse}[...](...) functions returning bool
    Add [[nodiscard] (or warn_unused_result) to all {Decode,Parse}[...](...) functions returning bool
    on Aug 2, 2018
  29. practicalswift renamed this:
    Add [[nodiscard] (or warn_unused_result) to all {Decode,Parse}[...](...) functions returning bool
    Add [[nodiscard]] (or warn_unused_result) to all {Decode,Parse}[...](...) functions returning bool
    on Aug 2, 2018
  30. practicalswift commented at 3:57 PM on August 2, 2018: contributor

    What about the following?

    $ cat src/attributes.h
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-2018 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_ATTRIBUTES_H
#define BITCOIN_ATTRIBUTES_H

#ifdef __has_cpp_attribute
#  if __has_cpp_attribute(nodiscard)
#    define NODISCARD [[nodiscard]]
#  endif
#else
#  define NODISCARD __attribute__((warn_unused_result))
#endif

#endif // BITCOIN_ATTRIBUTES_H
  31. practicalswift force-pushed on Aug 2, 2018
  32. practicalswift commented at 4:21 PM on August 2, 2018: contributor

    Now using NODISCARD ([[nodiscard]] or __attribute__((warn_unused_result))) throughout. Please re-review :-)

  33. practicalswift force-pushed on Aug 2, 2018
  34. Empact commented at 6:40 PM on August 2, 2018: member

    Concept ACK, and loving NODISCARD. But you need to add attributes.h to src/Makefile.am. Also please sort your new include alphabetically.

  35. in src/attributes.h:10 in 1856b807d1 outdated 
       5 | +
   6 | +#ifndef BITCOIN_ATTRIBUTES_H
   7 | +#define BITCOIN_ATTRIBUTES_H
   8 | +
   9 | +#ifdef __has_cpp_attribute
  10 | +#  if __has_cpp_attribute(nodiscard)
    Empact commented at 6:44 PM on August 2, 2018:

    You should collapse this to one level: #if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard)

    Otherwise NODISCARD will be undefined if __has_cpp_attribute(nodiscard) is false.

  36. practicalswift force-pushed on Aug 3, 2018
  37. practicalswift commented at 7:30 AM on August 3, 2018: contributor

    @Empact Thanks for reviewing! Feedback addressed. The broken ifdef logic was sloppy from my side – sorry about that. Would you mind re-reviewing? :-)

  38. practicalswift renamed this:
    Add [[nodiscard]] (or warn_unused_result) to all {Decode,Parse}[...](...) functions returning bool
    util: Add [[nodiscard]] (or warn_unused_result) to all {Decode,Parse}[...](...) functions returning bool
    on Aug 3, 2018
  39. practicalswift renamed this:
    util: Add [[nodiscard]] (or warn_unused_result) to all {Decode,Parse}[...](...) functions returning bool
    util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool
    on Aug 3, 2018
  40. Empact commented at 3:00 PM on August 3, 2018: member

    On second thought, how about throw rather than default on unparsable option? Seems like a failure to be noisy about.

  41. skeees commented at 3:00 PM on August 3, 2018: contributor

    In terms of commit ordering, you need the #define NODISCARD commit to precede the commits where you use NODISCARD Other than that lgtm!

  42. DrahtBot added the label Needs rebase on Aug 15, 2018
  43. practicalswift force-pushed on Aug 15, 2018
  44. practicalswift commented at 8:36 PM on August 15, 2018: contributor

    Rebased! :-) @MarcoFalke I don't know if I've thanked you before but thanks for your work on @DrahtBot! It really helps keeping the PR:s up to date! Really nice!

  45. DrahtBot removed the label Needs rebase on Aug 15, 2018
  46. practicalswift force-pushed on Aug 27, 2018
  47. practicalswift commented at 5:15 PM on August 27, 2018: contributor

    Updated NODISCARD to work also with Visual C++ :-)

  48. DrahtBot added the label Needs rebase on Aug 28, 2018
  49. practicalswift force-pushed on Aug 28, 2018
  50. practicalswift commented at 8:58 PM on August 28, 2018: contributor

    Rebased!

  51. practicalswift force-pushed on Aug 28, 2018
  52. practicalswift force-pushed on Aug 28, 2018
  53. practicalswift force-pushed on Aug 28, 2018
  54. DrahtBot removed the label Needs rebase on Aug 28, 2018
  55. practicalswift force-pushed on Aug 28, 2018
  56. in src/miner.cpp:74 in da78f95f5a outdated 
      69 | @@ -70,9 +70,8 @@ static BlockAssembler::Options DefaultOptions()
  70 |      // If -blockmaxweight is not given, limit to DEFAULT_BLOCK_MAX_WEIGHT
  71 |      BlockAssembler::Options options;
  72 |      options.nBlockMaxWeight = gArgs.GetArg("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT);
  73 | -    if (gArgs.IsArgSet("-blockmintxfee")) {
  74 | -        CAmount n = 0;
  75 | -        ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n);
  76 | +    CAmount n = 0;
  77 | +    if (gArgs.IsArgSet("-blockmintxfee") && ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) {
    arvidn commented at 2:23 PM on September 17, 2018:

    with this change the scope of variable n expands and "leaks" outside of where it's used. perhaps not a big deal, but if we allow C++17 this could be written as:

    if (CAmount n = 0; gArgs.IsArgSet("-blockmintxfee") && ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) {

    practicalswift commented at 2:31 PM on September 17, 2018:

    @arvidn We allow only C++11 :-)

  57. in src/rest.cpp:109 in da78f95f5a outdated 
     105 | @@ -105,7 +106,7 @@ static std::string AvailableDataFormatsString()
 106 |      return formats;
 107 |  }
 108 |  
 109 | -static bool ParseHashStr(const std::string& strReq, uint256& v)
 110 | +NODISCARD static bool ParseHashStr(const std::string& strReq, uint256& v)
    arvidn commented at 2:24 PM on September 17, 2018:

    I would expect you don't need this attribute on the function definition, but just the declaration. Do you really need it here too?

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

    ditto for all other definitions

    Empact commented at 4:40 AM on September 25, 2018:

    This is misleading because it is actually a static function, there is no declaration, though there is another identically-named function. See #14307

  58. in src/script/descriptor.cpp:434 in da78f95f5a outdated 
     430 | @@ -431,7 +431,7 @@ std::vector<Span<const char>> Split(const Span<const char>& sp, char sep)
 431 |  }
 432 |  
 433 |  /** Parse a key path, being passed a split list of elements (the first element is ignored). */
 434 | -bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath& out)
 435 | +NODISCARD static bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath& out)
    arvidn commented at 2:26 PM on September 17, 2018:

    in C++ it's generally preferred to putt functions with internal linkage in the anonymous namespace instead of using static, but I see a lot of static, so perhaps that's the style here

    practicalswift commented at 12:16 PM on October 7, 2018:

    Yes, going with static here to keep the number of lines touched lines to a minimum.

    practicalswift commented at 10:27 AM on October 9, 2018:

    Oh, turns out ParseKeyPath(…) is already in the anonymous namespace. Thus making static redundant. Now removed. Thanks for the nit @arvidn!

  59. in src/utilstrencodings.h:41 in da78f95f5a outdated 
      36 | @@ -35,8 +37,8 @@ enum SafeChars
  37 |  * @return           A new string without unsafe chars
  38 |  */
  39 |  std::string SanitizeString(const std::string& str, int rule = SAFE_CHARS_DEFAULT);
  40 | -std::vector<unsigned char> ParseHex(const char* psz);
  41 | -std::vector<unsigned char> ParseHex(const std::string& str);
  42 | +NODISCARD std::vector<unsigned char> ParseHex(const char* psz);
  43 | +NODISCARD std::vector<unsigned char> ParseHex(const std::string& str);
    Empact commented at 4:49 AM on September 25, 2018:

    These don't return bool

  60. Empact commented at 4:51 AM on September 25, 2018: member

    I see apparently unnecessary changes to: src/wallet/rpcwallet.cpp src/wallet/test/psbt_wallet_tests.cpp

  61. practicalswift force-pushed on Sep 25, 2018
  62. practicalswift commented at 5:02 AM on September 25, 2018: contributor

    @Empact Unrelated sorting of includes in those two files removed :-)

  63. in src/Makefile.am:98 in 58cee45c23 outdated 
      91 | @@ -92,6 +92,7 @@ endif
  92 |  BITCOIN_CORE_H = \
  93 |    addrdb.h \
  94 |    addrman.h \
  95 | +  attributes.h \
    ken2812221 commented at 5:35 AM on September 25, 2018:

    Wrong file, should be nodiscard.h?

    practicalswift commented at 7:28 AM on September 25, 2018:

    attributes.h is correct but that file got lost in the last rebase. Now re-added :-)

  64. in src/base58.h:17 in e6d8872366 outdated 
      13 | @@ -14,6 +14,8 @@
  14 |  #ifndef BITCOIN_BASE58_H
  15 |  #define BITCOIN_BASE58_H
  16 |  
  17 | +#include <attributes.h>
    ken2812221 commented at 5:40 AM on September 25, 2018:

    Same here

    practicalswift commented at 7:28 AM on September 25, 2018:

    attributes.h is correct but that file got lost in the last rebase. Now re-added :-)

    ken2812221 commented at 7:35 AM on September 25, 2018:

    Seems attributes.h is as same as nodiscard.h now

    practicalswift commented at 7:45 AM on September 25, 2018:

    nodiscard.h now removed :-)

  65. practicalswift force-pushed on Sep 25, 2018
  66. practicalswift force-pushed on Sep 25, 2018
  67. DrahtBot added the label Needs rebase on Sep 27, 2018
  68. practicalswift force-pushed on Oct 7, 2018
  69. practicalswift commented at 12:18 PM on October 7, 2018: contributor

    Rebased! Please re-review @ken2812221, @arvidn, @Empact, @skeees and @fanquake :-)

  70. DrahtBot removed the label Needs rebase on Oct 7, 2018
  71. practicalswift commented at 10:27 AM on October 9, 2018: contributor

    Updated to address @arvidn:s static nit. Please re-review :-)

  72. in src/attributes.h:21 in ee4b5b8e6d outdated 
      11 | +#elif defined(_MSC_VER) && _MSC_VER >= 1700
  12 | +#  define NODISCARD _Check_return_
  13 | +#else
  14 | +#  define NODISCARD __attribute__((warn_unused_result))
  15 | +#endif
  16 | +
    ken2812221 commented at 10:53 AM on October 9, 2018:

    This solution might be too complex, but it can get rid of annoying MSVC warnings.

    #if defined(__has_cpp_attribute)
#  if __has_cpp_attribute(nodiscard)
#    define NODISCARD [[nodiscard]]
#  endif
#endif
#ifndef NODISCARD
#  if defined(_MSC_VER) && _MSC_VER >= 1700
#    define NODISCARD _Check_return_
#  else
#    define NODISCARD __attribute__((warn_unused_result))
#  endif
#endif
    practicalswift commented at 10:19 AM on October 11, 2018:

    Fixed!

  73. practicalswift force-pushed on Oct 11, 2018
  74. practicalswift force-pushed on Oct 11, 2018
  75. practicalswift force-pushed on Oct 11, 2018
  76. practicalswift commented at 10:20 AM on October 11, 2018: contributor

    Updated to address MSVC warnings. Please re-review :-)

  77. practicalswift force-pushed on Oct 11, 2018
  78. practicalswift commented at 8:22 PM on October 11, 2018: contributor

    @ken2812221 Seems like I'm unable to get rid of the ugly MSVC warnings ("unexpected tokens following preprocessor directive […]"). Do you have any theory on how to fix it? I don't have MSVC available – only AppVeyor :-)

  79. ken2812221 commented at 8:25 PM on October 11, 2018: contributor

    Just put __has_cpp_attribute(...) inside the condition #ifdef __has_cpp_attribute. Like what I posted.

  80. practicalswift force-pushed on Oct 11, 2018
  81. DrahtBot added the label Needs rebase on Nov 5, 2018
  82. practicalswift force-pushed on Nov 5, 2018
  83. practicalswift commented at 12:54 PM on November 5, 2018: contributor

    Rebased!

  84. DrahtBot removed the label Needs rebase on Nov 5, 2018
  85. miner: Default to DEFAULT_BLOCK_MIN_TX_FEE if unable to parse -blockmintxfee 7c5bc2a523
  86. tests: Check return value of ParseParameters(...) 145fe95ec7
  87. tests: Explicitly ignore the return value of DecodeBase58(...) 579497e77a
  88. Add NODISCARD to all {Decode,Parse}[...](...) functions returning bool. Sort includes. 9cc0230cfc
  89. practicalswift force-pushed on Nov 5, 2018
  90. MarcoFalke commented at 8:43 PM on November 8, 2018: member

    utACK 9cc0230cfc1ae9b9c1c905cd9ac613bc98bfa43a

  91. Empact commented at 9:05 PM on November 8, 2018: member

    utACK 9cc0230

  92. sipa commented at 11:05 PM on November 14, 2018: member

    utACK 9cc0230cfc1ae9b9c1c905cd9ac613bc98bfa43a

  93. MarcoFalke merged this on Nov 15, 2018
  94. MarcoFalke closed this on Nov 15, 2018
  95. MarcoFalke referenced this in commit 384967f311 on Nov 15, 2018
  96. kittywhiskers referenced this in commit c6f397067b on Mar 10, 2021
  97. kittywhiskers referenced this in commit bf3997c7d4 on Mar 10, 2021
  98. kittywhiskers referenced this in commit 38ef8ef742 on Mar 10, 2021
  99. kittywhiskers referenced this in commit 6daf802aae on Mar 10, 2021
  100. kittywhiskers referenced this in commit edbf85f003 on Mar 10, 2021
  101. kittywhiskers referenced this in commit 3600feee2e on Mar 10, 2021
  102. kittywhiskers referenced this in commit 0e6fdcf0e5 on Mar 10, 2021
  103. kittywhiskers referenced this in commit 2380070b2e on Mar 12, 2021
  104. kittywhiskers referenced this in commit c0845e585f on Mar 12, 2021
  105. kittywhiskers referenced this in commit bd195f00f5 on Mar 12, 2021
  106. kittywhiskers referenced this in commit eb3511395a on Mar 19, 2021
  107. PastaPastaPasta referenced this in commit e20dc9f374 on Mar 22, 2021
  108. practicalswift deleted the branch on Apr 10, 2021
  109. random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
  110. gades referenced this in commit 0647f7b2bf on Mar 16, 2022
  111. DrahtBot locked this on Aug 16, 2022
Contributors
practicalswiftfanquakeEmpactskeeesDrahtBotarvidnken2812221MarcoFalkesipa
Labels
Scripts and tools
Linked (view graph)
#13712 wallet: Fix non-determinism in ParseHDKeypath(...). Avoid using an uninitialized variable in path calculation.
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-30 03:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me