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

1. 
   laanwj commented at 12:34 pm on October 28, 2019: member

   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)

2. refactor: Remove redundant c_str() calls in formatting

   Our formatter, tinyformat, *never* needs `c_str()` for strings.
   Remove redundant `c_str()` calls for:
   
   - `strprintf`
   - `LogPrintf`
   - `tfm::format`

   c72906dcc1
3. laanwj added the label Refactoring on Oct 28, 2019
4. 
   DrahtBot commented at 12:54 pm on October 28, 2019: member

   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.

5. 
   fanquake commented at 1:42 pm on October 28, 2019: member

   Looks like there is still a usage of c_str() with LogPrintf in wallet.h: https://github.com/bitcoin/bitcoin/blob/1ab6c1267e342e034758d4c18e0b3291705d7edb/src/wallet/wallet.h#L1412
6. 
   laanwj commented at 1:54 pm on October 28, 2019: member

   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 :smile:
7. 
   practicalswift commented at 2:30 pm on October 28, 2019: contributor

   Concept ACK: nice cleanup
8. 
   ryanofsky commented at 4:24 pm on October 28, 2019: member

   Code review ACK c72906dcc11a73fa06a0adf97557fa756b551bee. Easy to review with git log -p -n1 --word-diff-regex=. -U0 c72906dcc11a73fa06a0adf97557fa756b551bee
9. 
   MarcoFalke commented at 7:08 pm on October 28, 2019: member

   ACK, the args are:

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

10. MarcoFalke referenced this in commit 4c1090c882 on Oct 28, 2019
11. MarcoFalke merged this on Oct 28, 2019
12. MarcoFalke closed this on Oct 28, 2019

    ---

13. 
    MarcoFalke commented at 7:13 pm on October 28, 2019: member

    Can we make it a compile time error to pass c-strings in to format? Passing them in should never be necessary.
14. 
    MarcoFalke commented at 7:25 pm on October 28, 2019: member

    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:

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

15. 
    laanwj commented at 7:29 pm on October 28, 2019: member

    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.

16. deadalnix referenced this in commit dcc4c33d7c on Apr 30, 2020
17. MarcoFalke locked this on Dec 16, 2021

Contributors
laanwj DrahtBot fanquake practicalswift ryanofsky MarcoFalke

Labels
Refactoring