Skip to content

refactor: Remove redundant c_str() calls in formatting #17279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maflcko merged 1 commit into from
Oct 28, 2019
Merged

refactor: Remove redundant c_str() calls in formatting #17279

maflcko merged 1 commit into from
Oct 28, 2019

Conversation

@laanwj
Copy link
Member

@laanwj laanwj commented Oct 28, 2019 •
edited
Loading

Our formatter, tinyformat, never needs c_str() for strings. Still, many places call it redundantly, resulting in longer code and a slight overhead.

Remove redundant c_str() calls for:

  • strprintf
  • LogPrintf
  • tfm::format

(also, combined with #17095, I think this improves logging in case of unexpected embedded NULL characters)

@laanwj
refactor: Remove redundant c_str() calls in formatting
c72906d 
Our formatter, tinyformat, *never* needs `c_str()` for strings.
Remove redundant `c_str()` calls for:

- `strprintf`
- `LogPrintf`
- `tfm::format`
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 28, 2019 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16545 (Implement missing error checking for ArgsManager flags by ryanofsky)
  • #16224 (gui: Bilingual GUI error messages by hebasto)
  • #15934 (Merge settings one place instead of five places by ryanofsky)
  • #14866 (Improve property evaluation way in bitcoin.conf by AkioNak)

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.

@fanquake
Copy link
Member

fanquake commented Oct 28, 2019

Looks like there is still a usage of c_str() with LogPrintf in wallet.h:

bitcoin/src/wallet/wallet.h

Line 1412 in 1ab6c12

LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...);

@laanwj
Copy link
Member Author

laanwj commented Oct 28, 2019

That one is special: it adds a prefix to the format string, which is a c_str(). It's something not normally allowed, and we even have an exception in the linter for it 😄

@practicalswift
Copy link
Contributor

practicalswift commented Oct 28, 2019

Concept ACK: nice cleanup

@ryanofsky
Copy link
Contributor

ryanofsky commented Oct 28, 2019

Code review ACK c72906d. Easy to review with git log -p -n1 --word-diff-regex=. -U0 c72906dcc11a73fa06a0adf97557fa756b551bee

@maflcko
Copy link
Member

maflcko commented Oct 28, 2019

ACK, the args are:

std::string format(const char* formatString, const Args&... args);

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 28, 2019
Merge bitcoin#17279: refactor: Remove redundant c_str() calls in form…
4c1090c 
…atting

c72906d refactor: Remove redundant c_str() calls in formatting (Wladimir J. van der Laan)

Pull request description:

  Our formatter, tinyformat, *never* needs `c_str()` for strings. Still, many places call it redundantly, resulting in longer code and a slight overhead.

  Remove redundant `c_str()` calls for:

  - `strprintf`
  - `LogPrintf`
  - `tfm::format`

  (also, combined with bitcoin#17095, I think this improves logging in case of unexpected embedded NULL characters)

ACKs for top commit:
  ryanofsky:
    Code review ACK c72906d. Easy to review with `git log -p -n1 --word-diff-regex=. -U0 c72906d`

Tree-SHA512: 9e21e7bed8aaff59b8b8aa11571396ddc265fb29608c2545b1fcdbbb36d65b37eb361db6688dd36035eab0c110f8de255375cfda50df3d9d7708bc092f67fefc
@maflcko maflcko merged commit c72906d into bitcoin:master Oct 28, 2019
@maflcko
Copy link
Member

maflcko commented Oct 28, 2019

Can we make it a compile time error to pass c-strings in to format? Passing them in should never be necessary.

@maflcko
Copy link
Member

maflcko commented Oct 28, 2019

Can we make it a compile time error to pass c-strings in to format? Passing them in should never be necessary.

Oh, I guess that is not true. One might want to print "%s" verbose, like this:

tfm::format(std::cerr, "%s", "%s");

@laanwj
Copy link
Member Author

laanwj commented Oct 28, 2019

Can we make it a compile time error to pass c-strings in to format? Passing them in should never be necessary.

I don't think this is possible, as a function that only takes std::string will accept C string due to the implicit constructor.

@ryanofsky ryanofsky mentioned this pull request Oct 29, 2019
Merged
@hebasto hebasto mentioned this pull request Oct 29, 2019
Merged
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Apr 30, 2020
@laanwj @deadalnix
refactor: Remove redundant c_str() calls in formatting
dcc4c33 
Summary:
Our formatter, tinyformat, *never* needs `c_str()` for strings.
Remove redundant `c_str()` calls for:

- `strprintf`
- `LogPrintf`
- `tfm::format`

This is a backport of Core [[bitcoin/bitcoin#17279 | PR17279]]

Test Plan:
  ninja all check

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5908
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@laanwj @DrahtBot @fanquake @practicalswift @ryanofsky @maflcko