refactor: Replace const char* to std::string

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:

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

DrahtBot added the label Consensus on May 18, 2020

DrahtBot added the label Tests on May 18, 2020

MarcoFalke removed the label Tests on May 18, 2020

MarcoFalke added the label Refactoring on May 18, 2020

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

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

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.

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?

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.

kcalvinalvin commented at 12:41 pm on May 18, 2020: contributor

Done :)

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

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

practicalswift commented at 2:01 pm on May 18, 2020: contributor

Concept ACK modulo nits

Welcome as a contributor @kcalvinalvin :)

hebasto commented at 2:16 pm on May 18, 2020: member

Concept ACK.

kcalvinalvin force-pushed on May 18, 2020

kcalvinalvin commented at 4:07 pm on May 18, 2020: contributor

Addressed GetTxnOutputType() in the new commit as requested here

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.

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

kcalvinalvin force-pushed on May 18, 2020

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

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.

tryphe commented at 5:35 pm on May 18, 2020: contributor

Concept ACK

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

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

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

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

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

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.

kcalvinalvin force-pushed on May 18, 2020

hebasto approved

hebasto commented at 10:48 am on May 20, 2020: member

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

MarcoFalke commented at 12:08 pm on May 20, 2020: member

ACK bdd9cb6a9bc314400ad655a45ee32e03756468d1 🥑

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK bdd9cb6a9bc314400ad655a45ee32e03756468d1 🥑
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhkggv/dG77SPx7h89oenzVpqi337dajwuIwkr5AzDinyxCC+s+ZuWqRXM84r5Y
 8Kp2dppe4rvo5XnR6EYPgdGsICBt1tEarppmtEExnYrUfB7UB2PSyqfZ+A1juurnj
 9BTzRo019nO/RLavDVqEq/aqSa8fW8mkTu2gO95XLXsWFONojQAe0qRrMCwAAiviB
10r54j6ApH2mJ+nllZqiI9yQxztUVFbIVWqZai+GIcy/wLMS6b6ssTMGH4y6NT1Ob4
11O1t7eNENIZLv4RnEddCckA24rxVDOPE49ArD62E/yZn/9uzHfC+ZaFcCTyZ9eH12
12h3bexZOOcnQes1nutjLYNVvG6+HAmaC3EjZuZhnX6wFvMUvrUzpCvbTe0WqRDy0u
13GVER/1LkC0o3I+/xUADGzOutgKbviv8nqP1/fjdfkRXSbEMJ0KUknT6GYxbEADBu
14NYf1BZdrK1hOmBQlO4N4upbt/UsnldngjW1xUPnCMtp9Ao+0le6qwzzZGC25qav6
15vMt9IqV2
16=535T
17-----END PGP SIGNATURE-----

Timestamp of file with hash 59bb0b7e1b6d4de434d0c1c688938d18335cc4075bc7302fd2f2754cdc746917 -

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:

0bdd9cb6a9bc314400ad655a45ee32e03756468d1 (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.

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: ?

refactor: Replace const char* to std::string

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

c57f03ce17

kcalvinalvin force-pushed on May 21, 2020

kcalvinalvin renamed this:
~~script: Replace const char* to std::string~~
refactor: Replace const char* to std::string
on May 21, 2020

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:

hebasto approved

hebasto commented at 4:45 pm on May 21, 2020: member

re-ACK c57f03ce1741b38af448bec7b22ab9f8ac21f067

MarcoFalke commented at 4:45 pm on May 21, 2020: member

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

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK c57f03ce17 (no changes since previous review) 🚃
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUg5Ugv/RLY5cmEYDu4MGRx80v7YSnQnpIxJ2l2+ls+NQLYL6wCj5oOG/g67mSvW
 8UCmzq51vj0FRhZJ+Kz54WUpHlizyWaettFcpytCug+O5u+7iWU5xwusrZTZxIIMq
 9hySm3NdHELk47dI2BTYAYtosJfEizyee/FimSovaWbokufmybwxXsjkMwz+NfeGv
10D5qn0dh04Vs9y9GiN+9eSLMZn2b6tt7Wv5ryNjb+pNI7DzOfL2wiRqwGGRp+dAmO
11cNcRzVxqmDXj+a23kiqMB7v9QujylftRsNnJ9gRXhNUDMxdGFRMG+tqcFKSWs/cU
123SI/n5y2ostgpVdFKaeKP56fiut0YUxKSrvlCZah3QIiEKMPiafILz1q8naN+Z5J
13FxwIV0gpUBewkvBNv8Mmbi3c7bHAgNXHDr9XpWYq2N0wVgMjzv+bf2p55PlLi6HC
14oU1IGMAMBSfLAzjxbhMQH1GBSdRX2NKqADGip4YlaXYiX4gYg8QQ/F9RPrVK1xZS
15ohz+lirt
16=GHzP
17-----END PGP SIGNATURE-----

Timestamp of file with hash d852b864158d4b33654116fe600d925926dc1f39c2c521efb25067754e5f918d -

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

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.

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

practicalswift commented at 9:48 am on May 27, 2020: contributor

ACK c57f03ce1741b38af448bec7b22ab9f8ac21f067 – patch looks correct

MarcoFalke merged this on May 27, 2020

MarcoFalke closed this on May 27, 2020

sidhujag referenced this in commit 3290d4fb37 on May 27, 2020

Fabcien referenced this in commit d56bef9201 on Feb 2, 2021

DrahtBot locked this on Feb 15, 2022

kcalvinalvin deleted the branch on Jul 13, 2022

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