Investigate where std::string should be returned instead of "const char*"

practicalswift commented at 8:33 pm on May 17, 2020: contributor

As noted by @MarcoFalke in #18994 (review) the case can be made that at least some of the functions in our code base returning const char* should return std::string instead.

Candidates:

0$ git grep -E '^(static |inline |const |unsigned )*char *\*[^(<]*\(' ":(exclude)src/leveldb/" ":(exclude)src/univalue/" ":(exclude)src/serialize.h" ":(exclude)src/tinyformat.h"
1src/bitcoin-cli.cpp:static const char *http_errorstring(int code)
2src/script/script.cpp:const char* GetOpName(opcodetype opcode)
3src/script/script.h:const char* GetOpName(opcodetype opcode);
4src/script/script_error.cpp:const char* ScriptErrorString(const ScriptError serror)
5src/script/script_error.h:const char* ScriptErrorString(const ScriptError error);
6src/script/standard.cpp:const char* GetTxnOutputType(txnouttype t)
7src/script/standard.h:const char* GetTxnOutputType(txnouttype t);
8src/test/script_tests.cpp:static const char *FormatScriptError(ScriptError_t err)

Want to work on this issue?

The purpose of the good first issue label is to highlight which issues are suitable for a new contributor without a deep understanding of the codebase.

You do not need to request permission to start working on this. You are encouraged to comment on the issue if you are planning to work on it. This will help other contributors monitor which issues are actively being addressed and is also an effective way to request assistance if and when you need it.

For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

practicalswift added the label good first issue on May 17, 2020

MarcoFalke commented at 12:19 pm on May 18, 2020: member

GetTxnOutputType returns a nullptr, but none of the call sites check for that

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

@MarcoFalke Yes, returning std::string{} (the empty string) is probably the right thing to do where we return nullptr today. @kcalvinalvin Would you mind tackling also GetTxnOutputType in your PR to make the change complete? :)

MarcoFalke commented at 2:41 pm on May 18, 2020: member

I don’t think it should return an empty string. Either the case can not be hit, so an assert (or static_assert) is appropriate. Or it can be hit, in which case it should return an Optional<>, to force the caller to check.

An empty string combines the worst of both worlds: It doesn’t ask the caller to check for the error condition or it silently pretends the case can not exist.

kcalvinalvin commented at 3:13 pm on May 18, 2020: contributor

@kcalvinalvin Would you mind tackling also GetTxnOutputType in your PR to make the change complete? :)

Sure I’ll get on that

practicalswift commented at 5:17 am on May 19, 2020: contributor

I don’t think it should return an empty string. Either the case can not be hit, so an assert (or static_assert) is appropriate. Or it can be hit, in which case it should return an Optional<>, to force the caller to check.

Good point. assert(false); is probably the right thing to do in this case.

MarcoFalke referenced this in commit 9ccaee1d5e on May 27, 2020

MarcoFalke closed this on May 27, 2020

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

DrahtBot locked this on Feb 15, 2022

Investigate where std::string should be returned instead of “const char*” #19000

Want to work on this issue?