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

kcalvinalvin · 2020-05-18T09:07:37Z

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

maflcko · 2020-05-18T10:38:51Z

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 · 2020-05-18T12:02:39Z

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

maflcko · 2020-05-18T12:17:26Z

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 · 2020-05-18T12:26:54Z

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?

maflcko · 2020-05-18T12:39:16Z

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 · 2020-05-18T12:41:24Z

Done :)

practicalswift

Concept ACK modulo nits

Welcome as a contributor @kcalvinalvin :)

src/core_read.cpp

hebasto · 2020-05-18T14:16:26Z

Concept ACK.

kcalvinalvin · 2020-05-18T16:07:10Z

Addressed GetTxnOutputType() in the new commit as requested here

src/core_write.cpp

src/script/standard.cpp

sipa · 2020-05-18T17:05:36Z

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

maflcko · 2020-05-18T17:08:10Z

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 · 2020-05-18T17:35:11Z

Concept ACK

src/script/standard.h

src/script/standard.cpp

src/script/script.cpp

src/core_read.cpp

src/bitcoin-cli.cpp

sipa · 2020-05-18T17:50:46Z

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

hebasto

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

maflcko · 2020-05-20T12:08:34Z

ACK bdd9cb6a9bc314400ad655a45ee32e03756468d1 🥑

Show signature and timestamp

Signature:

Timestamp of file with hash 59bb0b7e1b6d4de434d0c1c688938d18335cc4075bc7302fd2f2754cdc746917 -

laanwj · 2020-05-21T15:14:37Z

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.

hebasto · 2020-05-21T15:18:05Z

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

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

kcalvinalvin · 2020-05-21T16:43:45Z

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

re-ACK c57f03c

maflcko · 2020-05-21T16:45:59Z

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

Show signature and timestamp

Signature:

Timestamp of file with hash d852b864158d4b33654116fe600d925926dc1f39c2c521efb25067754e5f918d -

Empact · 2020-05-26T21:57:05Z

~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:
master...Empact:replace-char-with-string

maflcko · 2020-05-26T23:19:08Z

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 · 2020-05-26T23:21:51Z

Fair enough, Code Review ACK c57f03c

practicalswift · 2020-05-27T09:48:47Z

ACK c57f03c -- patch looks correct

c57f03c refactor: Replace const char* to std::string (Calvin Kim) Pull request description: Rationale: Addresses bitcoin#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 ACKs for top commit: MarcoFalke: re-ACK c57f03c (no changes since previous review) 🚃 Empact: Fair enough, Code Review ACK bitcoin@c57f03c practicalswift: ACK c57f03c -- patch looks correct hebasto: re-ACK c57f03c Tree-SHA512: 9ce99bb38fe399b54844315048204cafce0f27fd8f24cae357fa7ac6f5d8094d57bbf5f5c1f5878a65f2d35e4a3f95d527eb17f49250b690c591c0df86ca84fd

Summary: ``` Some functions should be returning std::string instead of const char*. This commit changes that. ``` Backport of core [[bitcoin/bitcoin#19004 | PR19004]]. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9124

DrahtBot added Consensus Tests labels May 18, 2020

maflcko added Refactoring and removed Tests labels May 18, 2020

practicalswift reviewed May 18, 2020

View reviewed changes

src/core_read.cpp Outdated Show resolved Hide resolved

src/core_read.cpp Outdated Show resolved Hide resolved

kcalvinalvin force-pushed the replace-char-with-string branch from 81d847c to a0da974 Compare May 18, 2020 16:04

hebasto reviewed May 18, 2020

View reviewed changes

src/core_write.cpp Outdated Show resolved Hide resolved

hebasto reviewed May 18, 2020

View reviewed changes

src/script/standard.cpp Outdated Show resolved Hide resolved

kcalvinalvin force-pushed the replace-char-with-string branch from a0da974 to 23610aa Compare May 18, 2020 16:48

tryphe reviewed May 18, 2020

View reviewed changes

src/script/standard.h Outdated Show resolved Hide resolved

src/script/standard.cpp Outdated Show resolved Hide resolved

src/script/script.cpp Outdated Show resolved Hide resolved

src/core_read.cpp Outdated Show resolved Hide resolved

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved

kcalvinalvin force-pushed the replace-char-with-string branch from 23610aa to bdd9cb6 Compare May 18, 2020 18:32

fanquake linked an issue May 19, 2020 that may be closed by this pull request

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

Closed

hebasto approved these changes May 20, 2020

View reviewed changes

refactor: Replace const char* to std::string

c57f03c

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

kcalvinalvin force-pushed the replace-char-with-string branch from bdd9cb6 to c57f03c Compare May 21, 2020 16:41

kcalvinalvin changed the title ~~script: Replace const char* to std::string~~ refactor: Replace const char* to std::string May 21, 2020

hebasto approved these changes May 21, 2020

View reviewed changes

maflcko merged commit 9ccaee1 into bitcoin:master May 27, 2020

furszy mentioned this pull request Sep 7, 2021

[Refactor] Differentiate destinations from lookup keys and disallow automatic conversion between them PIVX-Project/PIVX#2543

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

kcalvinalvin deleted the replace-char-with-string branch July 13, 2022 07:31

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

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

Uh oh!

Conversation

kcalvinalvin commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 18, 2020

Uh oh!

kcalvinalvin commented May 18, 2020

Uh oh!

maflcko commented May 18, 2020

Uh oh!

kcalvinalvin commented May 18, 2020

Uh oh!

maflcko commented May 18, 2020

Uh oh!

kcalvinalvin commented May 18, 2020

Uh oh!

practicalswift left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hebasto commented May 18, 2020

Uh oh!

kcalvinalvin commented May 18, 2020

Uh oh!

Uh oh!

Uh oh!

sipa commented May 18, 2020

Uh oh!

maflcko commented May 18, 2020

Uh oh!

tryphe commented May 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipa commented May 18, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 20, 2020

Uh oh!

laanwj commented May 21, 2020

Uh oh!

hebasto commented May 21, 2020

Uh oh!

kcalvinalvin commented May 21, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 21, 2020

Uh oh!

Empact commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 26, 2020

Uh oh!

Empact commented May 26, 2020

Uh oh!

practicalswift commented May 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

kcalvinalvin commented May 18, 2020 •

edited

Loading

Empact commented May 26, 2020 •

edited

Loading