refactor: Replace const char* to std::string #19004

pull kcalvinalvin wants to merge 1 commits into bitcoin:master from kcalvinalvin:replace-char-with-string changing 9 files +24 −13
  1. kcalvinalvin commented at 9:07 AM on May 18, 2020: contributor

    Rationale: Addresses #19000
    Some functions should be returning std::string instead of const char*. This commit changes that.

    Main benefits/reasoning:

    1. The functions never return nullptr, so returning a string makes code at call sites easier to review (reviewers don't have to read the source code to verify that a nullptr is never returned)
    2. All call sites convert to string anyway
  2. DrahtBot added the label Consensus on May 18, 2020
  3. DrahtBot added the label Tests on May 18, 2020
  4. MarcoFalke removed the label Tests on May 18, 2020
  5. MarcoFalke added the label Refactoring on May 18, 2020
  6. MarcoFalke commented at 10:38 AM on May 18, 2020: member

    Concept ACK. It would be nice to motivate the change in the opening post a bit:

    • The functions never return nullptr, so returning a string makes code at call sites easier to review (reviewers don't have to read the source code to verify that a nullptr is never returned)
    • All call sites convert to string anyway
  7. kcalvinalvin commented at 12:02 PM on May 18, 2020: contributor

    Concept ACK. It would be nice to motivate the change in the opening post a bit:

    * The functions never return nullptr, so returning a string makes code at call sites easier to review (reviewers don't have to read the source code to verify that a nullptr is never returned)

* All call sites convert to string anyway

    Thanks for the comment! Added those bits in the post

  8. MarcoFalke commented at 12:17 PM on May 18, 2020: member

    Also you can leave out the listing of files that you changed and what changes you made to those files. This is clear from reading the diff/looking at the changed files.

  9. kcalvinalvin commented at 12:26 PM on May 18, 2020: contributor

    Also you can leave out the listing of files that you changed and what changes you made to those files. This is clear from reading the diff/looking at the changed files.

    Ok. Should I take those out or just note for future PRs?

  10. MarcoFalke commented at 12:39 PM on May 18, 2020: member

    Both a suggestion for future PRs as well as this one, because the pull request description will end up in the merge commit, if this is merged.

  11. kcalvinalvin commented at 12:41 PM on May 18, 2020: contributor

    Done :)

  12. in src/core_read.cpp:37 in 81d847c389 outdated 
      33 | @@ -34,14 +34,13 @@ CScript ParseScript(const std::string& s)
  34 |              if (op < OP_NOP && op != OP_RESERVED)
  35 |                  continue;
  36 |  
  37 | -            const char* name = GetOpName(static_cast<opcodetype>(op));
  38 | -            if (strcmp(name, "OP_UNKNOWN") == 0)
  39 | +            std::string name = GetOpName(static_cast<opcodetype>(op));
    practicalswift commented at 1:55 PM on May 18, 2020:

    To minimize the diff consider calling the variable strName: const std::string strName = GetOpName(static_cast<opcodetype>(op)); perhaps? :)

    kcalvinalvin commented at 4:05 PM on May 18, 2020:

    Addressed in the new commit

  13. in src/core_read.cpp:38 in 81d847c389 outdated 
      33 | @@ -34,14 +34,13 @@ CScript ParseScript(const std::string& s)
  34 |              if (op < OP_NOP && op != OP_RESERVED)
  35 |                  continue;
  36 |  
  37 | -            const char* name = GetOpName(static_cast<opcodetype>(op));
  38 | -            if (strcmp(name, "OP_UNKNOWN") == 0)
  39 | +            std::string name = GetOpName(static_cast<opcodetype>(op));
  40 | +            if (name.compare("OP_UNKNOWN") == 0)
    practicalswift commented at 1:56 PM on May 18, 2020:

    I suggest doing this as if (name == "OP_UNKNOWN") instead to make it more idiomatic and thus easier for others to review and reason about :)

    kcalvinalvin commented at 4:05 PM on May 18, 2020:

    Changed in the new commit

  14. practicalswift commented at 2:01 PM on May 18, 2020: contributor

    Concept ACK modulo nits

    Welcome as a contributor @kcalvinalvin :)

  15. hebasto commented at 2:16 PM on May 18, 2020: member

    Concept ACK.

  16. kcalvinalvin force-pushed on May 18, 2020
  17. kcalvinalvin commented at 4:07 PM on May 18, 2020: contributor

    Addressed GetTxnOutputType() in the new commit as requested here

  18. in src/core_write.cpp:44 in a0da974a33 outdated 
      40 | @@ -41,7 +41,7 @@ std::string FormatScript(const CScript& script)
  41 |                  ret += strprintf("%i ", op - OP_1NEGATE - 1);
  42 |                  continue;
  43 |              } else if (op >= OP_NOP && op <= OP_NOP10) {
  44 | -                std::string str(GetOpName(op));
  45 | +                std::string str = GetOpName(op);
    hebasto commented at 4:18 PM on May 18, 2020:

    Why this change is needed?

    kcalvinalvin commented at 4:30 PM on May 18, 2020:

    I don't think it's needed. I'll revert the change.

  19. in src/script/standard.cpp:30 in a0da974a33 outdated 
      24 | @@ -25,7 +25,7 @@ WitnessV0ScriptHash::WitnessV0ScriptHash(const CScript& in)
  25 |      CSHA256().Write(in.data(), in.size()).Finalize(begin());
  26 |  }
  27 |  
  28 | -const char* GetTxnOutputType(txnouttype t)
  29 | +std::string GetTxnOutputType(txnouttype t)
    hebasto commented at 4:20 PM on May 18, 2020:

    This function has return nullptr; statement. It seems wrong :)

    kcalvinalvin commented at 4:29 PM on May 18, 2020:

    Yeah should be changed. Though I'm not sure it should be an assert or if it should return an Optional as stated here

    MarcoFalke commented at 4:34 PM on May 18, 2020:

    An assert seems appropriate, as this is unreachable code

    hebasto commented at 4:34 PM on May 18, 2020:

    Developer Notes - C++ data structures could be useful :)

    kcalvinalvin commented at 4:49 PM on May 18, 2020:

    Changed to assert

  20. kcalvinalvin force-pushed on May 18, 2020
  21. sipa commented at 5:05 PM on May 18, 2020: member

    It's unfortunate to make these functions return an std::string by value, as it implies a string copy every time they are called. This could be avoided by having a static const std::string inside the function for every value, and returning them as const std::string&.

  22. MarcoFalke commented at 5:08 PM on May 18, 2020: member

    All callers put the return value into a string anyway. So return value optimization (or whatever it is called) should take care of this and make it a no-op, no?

    Going forward, with C++17 they can be made constexpr string views if performance is really a concern.

  23. tryphe commented at 5:35 PM on May 18, 2020: contributor

    Concept ACK

  24. in src/script/standard.h:151 in 23610aa032 outdated 
     145 | @@ -146,7 +146,7 @@ typedef boost::variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash,
 146 |  bool IsValidDestination(const CTxDestination& dest);
 147 |  
 148 |  /** Get the name of a txnouttype as a C string, or nullptr if unknown. */
 149 | -const char* GetTxnOutputType(txnouttype t);
 150 | +std::string GetTxnOutputType(txnouttype t);
    tryphe commented at 5:42 PM on May 18, 2020:

    #include <string>

    kcalvinalvin commented at 6:01 PM on May 18, 2020:

    I've purposely only put #include <string> where the compiler yells at me since there are other .h and .cpp files that use std::string but omit #include <string> since there were some other file that had it.

    Not quite sure which I should follow...

    sipa commented at 6:03 PM on May 18, 2020:

    You should include whatever you use in that module. There are certainly existing violations in the codebase, but that's not guidance.

    kcalvinalvin commented at 6:32 PM on May 18, 2020:

    Thanks for the input. Added #include <string>

  25. in src/script/standard.cpp:29 in 23610aa032 outdated 
      24 | @@ -25,7 +25,7 @@ WitnessV0ScriptHash::WitnessV0ScriptHash(const CScript& in)
  25 |      CSHA256().Write(in.data(), in.size()).Finalize(begin());
  26 |  }
  27 |
    tryphe commented at 5:43 PM on May 18, 2020:

    #include <string> :)

    kcalvinalvin commented at 6:32 PM on May 18, 2020:

    addressed

  26. in src/script/script.cpp:12 in 23610aa032 outdated 
       6 | @@ -7,7 +7,7 @@
   7 |  
   8 |  #include <util/strencodings.h>
   9 |  
  10 | -const char* GetOpName(opcodetype opcode)
  11 | +std::string GetOpName(opcodetype opcode)
    tryphe commented at 5:44 PM on May 18, 2020:

    #include <string>

    kcalvinalvin commented at 6:32 PM on May 18, 2020:

    addressed

  27. in src/core_read.cpp:38 in 23610aa032 outdated 
      33 | @@ -34,10 +34,9 @@ CScript ParseScript(const std::string& s)
  34 |              if (op < OP_NOP && op != OP_RESERVED)
  35 |                  continue;
  36 |  
  37 | -            const char* name = GetOpName(static_cast<opcodetype>(op));
  38 | -            if (strcmp(name, "OP_UNKNOWN") == 0)
  39 | +            std::string strName = GetOpName(static_cast<opcodetype>(op));
    tryphe commented at 5:45 PM on May 18, 2020:

    #include <string> (it's not included but should have been before anyhow)

    kcalvinalvin commented at 6:33 PM on May 18, 2020:

    addressed

  28. in src/bitcoin-cli.cpp:161 in 23610aa032 outdated 
     156 | @@ -157,7 +157,7 @@ struct HTTPReply
 157 |      std::string body;
 158 |  };
 159 |  
 160 | -static const char *http_errorstring(int code)
 161 | +static std::string http_errorstring(int code)
    tryphe commented at 5:45 PM on May 18, 2020:

    #include <string>

    kcalvinalvin commented at 6:33 PM on May 18, 2020:

    addressed

  29. sipa commented at 5:50 PM on May 18, 2020: member

    @MarcoFalke That's a good point - this PR isn't making anything worse in that case. It could be improved later, but no need add extra complexity for that now.

  30. kcalvinalvin force-pushed on May 18, 2020
  31. hebasto approved
  32. hebasto commented at 10:48 AM on May 20, 2020: member

    ACK bdd9cb6a9bc314400ad655a45ee32e03756468d1, compiled on Linux Mint 19.3 with GCC and Clang.

  33. MarcoFalke commented at 12:08 PM on May 20, 2020: member

    ACK bdd9cb6a9bc314400ad655a45ee32e03756468d1 🥑

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK bdd9cb6a9bc314400ad655a45ee32e03756468d1 🥑
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhkggv/dG77SPx7h89oenzVpqi337dajwuIwkr5AzDinyxCC+s+ZuWqRXM84r5Y
Kp2dppe4rvo5XnR6EYPgdGsICBt1tEarppmtEExnYrUfB7UB2PSyqfZ+A1juurnj
BTzRo019nO/RLavDVqEq/aqSa8fW8mkTu2gO95XLXsWFONojQAe0qRrMCwAAiviB
r54j6ApH2mJ+nllZqiI9yQxztUVFbIVWqZai+GIcy/wLMS6b6ssTMGH4y6NT1Ob4
O1t7eNENIZLv4RnEddCckA24rxVDOPE49ArD62E/yZn/9uzHfC+ZaFcCTyZ9eH12
h3bexZOOcnQes1nutjLYNVvG6+HAmaC3EjZuZhnX6wFvMUvrUzpCvbTe0WqRDy0u
GVER/1LkC0o3I+/xUADGzOutgKbviv8nqP1/fjdfkRXSbEMJ0KUknT6GYxbEADBu
NYf1BZdrK1hOmBQlO4N4upbt/UsnldngjW1xUPnCMtp9Ao+0le6qwzzZGC25qav6
vMt9IqV2
=535T
-----END PGP SIGNATURE-----

    Timestamp of file with hash 59bb0b7e1b6d4de434d0c1c688938d18335cc4075bc7302fd2f2754cdc746917 -

    </details>

  34. laanwj commented at 3:14 PM on May 21, 2020: member

    ACK on the changes.

    Can you please reformat your commit message though to put an empty line between the title and the body? Tools such as git log --pretty=oneline currently merge the entire description into the title, which is messy:

    bdd9cb6a9bc314400ad655a45ee32e03756468d1 (pull/19004/head) script: Replace const char* to std::string Some functions should be returning std::string instead of const char*. This commit changes that.
  35. hebasto commented at 3:18 PM on May 21, 2020: member

    Can you please reformat your commit message though to put an empty line between the title and the body?

    ... and replace script: prefix with refactor: ?

  36. refactor: Replace const char* to std::string
    Some functions should be returning std::string instead of const char*.
This commit changes that.
    c57f03ce17
  37. kcalvinalvin force-pushed on May 21, 2020
  38. kcalvinalvin renamed this:
    script: Replace const char* to std::string
    refactor: Replace const char* to std::string
    on May 21, 2020
  39. kcalvinalvin commented at 4:43 PM on May 21, 2020: contributor

    ACK on the changes.

    Can you please reformat your commit message though to put an empty line between the title and the body?

    Done :)

    ... and replace script: prefix with refactor: ?

    Also Done :)

    Also changed the GitHub PR title from script: prefix to refactor:

  40. hebasto approved
  41. hebasto commented at 4:45 PM on May 21, 2020: member

    re-ACK c57f03ce1741b38af448bec7b22ab9f8ac21f067

  42. MarcoFalke commented at 4:45 PM on May 21, 2020: member

    re-ACK c57f03ce17 (no changes since previous review) 🚃

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK c57f03ce17 (no changes since previous review) 🚃
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUg5Ugv/RLY5cmEYDu4MGRx80v7YSnQnpIxJ2l2+ls+NQLYL6wCj5oOG/g67mSvW
UCmzq51vj0FRhZJ+Kz54WUpHlizyWaettFcpytCug+O5u+7iWU5xwusrZTZxIIMq
hySm3NdHELk47dI2BTYAYtosJfEizyee/FimSovaWbokufmybwxXsjkMwz+NfeGv
D5qn0dh04Vs9y9GiN+9eSLMZn2b6tt7Wv5ryNjb+pNI7DzOfL2wiRqwGGRp+dAmO
cNcRzVxqmDXj+a23kiqMB7v9QujylftRsNnJ9gRXhNUDMxdGFRMG+tqcFKSWs/cU
3SI/n5y2ostgpVdFKaeKP56fiut0YUxKSrvlCZah3QIiEKMPiafILz1q8naN+Z5J
FxwIV0gpUBewkvBNv8Mmbi3c7bHAgNXHDr9XpWYq2N0wVgMjzv+bf2p55PlLi6HC
oU1IGMAMBSfLAzjxbhMQH1GBSdRX2NKqADGip4YlaXYiX4gYg8QQ/F9RPrVK1xZS
ohz+lirt
=GHzP
-----END PGP SIGNATURE-----

    Timestamp of file with hash d852b864158d4b33654116fe600d925926dc1f39c2c521efb25067754e5f918d -

    </details>

  43. Empact commented at 9:57 PM on May 26, 2020: member

    ~0 I like all of these except GetOpName, which could return nullptr in place of "OP_UNKNOWN" - seems reasonable to minimize processing in the unknown case: https://github.com/bitcoin/bitcoin/compare/master...Empact:replace-char-with-string

  44. MarcoFalke commented at 11:19 PM on May 26, 2020: member

    Given that none of the call sites checked for nullptr of the char array, I'd find it very dangerous to re-introduce this sharp edge. Also for optional return values, we generally use Optional. In any case, I think this can be discussed in a follow-up pull request.

  45. Empact commented at 11:21 PM on May 26, 2020: member

    Fair enough, Code Review ACK https://github.com/bitcoin/bitcoin/pull/19004/commits/c57f03ce1741b38af448bec7b22ab9f8ac21f067

  46. practicalswift commented at 9:48 AM on May 27, 2020: contributor

    ACK c57f03ce1741b38af448bec7b22ab9f8ac21f067 -- patch looks correct

  47. MarcoFalke merged this on May 27, 2020
  48. MarcoFalke closed this on May 27, 2020
  49. sidhujag referenced this in commit 3290d4fb37 on May 27, 2020
  50. Fabcien referenced this in commit d56bef9201 on Feb 2, 2021
  51. DrahtBot locked this on Feb 15, 2022
  52. kcalvinalvin deleted the branch on Jul 13, 2022
Contributors
kcalvinalvinMarcoFalkepracticalswifthebastosipatryphelaanwjEmpact
Labels
RefactoringConsensus
Linked (view graph)
#19000 Investigate where std::string should be returned instead of "const char*"
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-14 21:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me