Skip to content

Conversation

@laanwj
Copy link
Member

@laanwj laanwj commented Oct 28, 2019

Add a note when to use and when not to use c_str().

@laanwj laanwj added the Docs label Oct 28, 2019
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 217e4a5304e3a042eb4983b48823bcfb7cb010e3. All good notes.

@cvengler
Copy link
Contributor

ACK

laanwj added a commit that referenced this pull request Oct 30, 2019
… to data()

f3b51eb Fix occurences of c_str() used with size() to data() (Wladimir J. van der Laan)

Pull request description:

  Using `data()` better communicates the intent here.

  ~~Also, depending on how `c_str()` is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents (or even be inaccessible, worst case).~~ Apparently [this is no longer an issue with C++11](#17281 (comment)).

ACKs for top commit:
  fjahr:
    Code review ACK f3b51eb
  practicalswift:
    ACK f3b51eb -- diff looks correct, `data()` more idiomatic
  ryanofsky:
    Code review ACK f3b51eb. Most of these calls (including one in crypter.cpp) are passing text strings, not binary strings likely to contain `\0` and were probably safe before, but much better to avoid the possibility of bugs like this.

Tree-SHA512: 842e1bdd37efc4ece2ecb87ca34962aafef0a192180051def630607e349dc9c8b4e562481fff3de474515f493b4ee3ea53b00269a801a66e625326a38dfce5b8
Add a note when to use and when not to use `c_str()`.
@laanwj laanwj force-pushed the 2019_10_c_str_note branch from 217e4a5 to 1cf9b35 Compare October 30, 2019 09:53
@laanwj
Copy link
Member Author

laanwj commented Oct 30, 2019

Addressed comments and squashed.

@elichai
Copy link
Contributor

elichai commented Oct 30, 2019

ACK 1cf9b35
I agree that unless you pass a string to C there's almost always a better way to do it in C++.
(and if you need array I agree with you that .data() is more self explanatory)

@maflcko
Copy link
Member

maflcko commented Oct 30, 2019

Looking nice ACK 1cf9b35

laanwj added a commit that referenced this pull request Oct 30, 2019
1cf9b35 doc: Add developer note on c_str() (Wladimir J. van der Laan)

Pull request description:

  Add a note when to use and when not to use `c_str()`.

ACKs for top commit:
  elichai:
    ACK 1cf9b35
  MarcoFalke:
    Looking nice ACK 1cf9b35

Tree-SHA512: 38cb5e54695782c23a82d03db214a8999b5bb52553f4fbe5322281686f42616981a217ba987feb6d87f3e6b95919cadd8484efe69ecc364ba1731aaf173626c9
@laanwj laanwj merged commit 1cf9b35 into bitcoin:master Oct 30, 2019
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 30, 2019
… size() to data()

f3b51eb Fix occurences of c_str() used with size() to data() (Wladimir J. van der Laan)

Pull request description:

  Using `data()` better communicates the intent here.

  ~~Also, depending on how `c_str()` is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents (or even be inaccessible, worst case).~~ Apparently [this is no longer an issue with C++11](bitcoin#17281 (comment)).

ACKs for top commit:
  fjahr:
    Code review ACK f3b51eb
  practicalswift:
    ACK f3b51eb -- diff looks correct, `data()` more idiomatic
  ryanofsky:
    Code review ACK f3b51eb. Most of these calls (including one in crypter.cpp) are passing text strings, not binary strings likely to contain `\0` and were probably safe before, but much better to avoid the possibility of bugs like this.

Tree-SHA512: 842e1bdd37efc4ece2ecb87ca34962aafef0a192180051def630607e349dc9c8b4e562481fff3de474515f493b4ee3ea53b00269a801a66e625326a38dfce5b8
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 31, 2019
1cf9b35 doc: Add developer note on c_str() (Wladimir J. van der Laan)

Pull request description:

  Add a note when to use and when not to use `c_str()`.

ACKs for top commit:
  elichai:
    ACK 1cf9b35
  MarcoFalke:
    Looking nice ACK 1cf9b35

Tree-SHA512: 38cb5e54695782c23a82d03db214a8999b5bb52553f4fbe5322281686f42616981a217ba987feb6d87f3e6b95919cadd8484efe69ecc364ba1731aaf173626c9

- *Rationale*: Bitcoin Core uses tinyformat, which is type safe. Leave them out to avoid confusion.

- Use `.c_str()` sparingly. Its only valid use is to pass C++ strings to C functions that take NULL-terminated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could say null-terminated instead of NULL-terminated. NULL constant is technically unrelated

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the NULL or NUL character, too. C's NULL constant is indeed unrelated.

jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 9, 2020
…e() to data()

Summary:
Fix occurences of c_str() used with size() to data() (Wladimir J. van der Laan)

Pull request description:

  Using `data()` better communicates the intent here.

  ~~Also, depending on how `c_str()` is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents (or even be inaccessible, worst case).~~ Apparently [this is no longer an issue with C++11](bitcoin/bitcoin#17281 (comment)).

---

Backport of Core [[bitcoin/bitcoin#17280 | PR17280]]

Test Plan:
  ninja check check-functional

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7387
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
… size() to data()

f3b51eb Fix occurences of c_str() used with size() to data() (Wladimir J. van der Laan)

Pull request description:

  Using `data()` better communicates the intent here.

  ~~Also, depending on how `c_str()` is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents (or even be inaccessible, worst case).~~ Apparently [this is no longer an issue with C++11](bitcoin#17281 (comment)).

ACKs for top commit:
  fjahr:
    Code review ACK f3b51eb
  practicalswift:
    ACK f3b51eb -- diff looks correct, `data()` more idiomatic
  ryanofsky:
    Code review ACK f3b51eb. Most of these calls (including one in crypter.cpp) are passing text strings, not binary strings likely to contain `\0` and were probably safe before, but much better to avoid the possibility of bugs like this.

Tree-SHA512: 842e1bdd37efc4ece2ecb87ca34962aafef0a192180051def630607e349dc9c8b4e562481fff3de474515f493b4ee3ea53b00269a801a66e625326a38dfce5b8
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
1cf9b35 doc: Add developer note on c_str() (Wladimir J. van der Laan)

Pull request description:

  Add a note when to use and when not to use `c_str()`.

ACKs for top commit:
  elichai:
    ACK 1cf9b35
  MarcoFalke:
    Looking nice ACK 1cf9b35

Tree-SHA512: 38cb5e54695782c23a82d03db214a8999b5bb52553f4fbe5322281686f42616981a217ba987feb6d87f3e6b95919cadd8484efe69ecc364ba1731aaf173626c9
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
1cf9b35 doc: Add developer note on c_str() (Wladimir J. van der Laan)

Pull request description:

  Add a note when to use and when not to use `c_str()`.

ACKs for top commit:
  elichai:
    ACK 1cf9b35
  MarcoFalke:
    Looking nice ACK 1cf9b35

Tree-SHA512: 38cb5e54695782c23a82d03db214a8999b5bb52553f4fbe5322281686f42616981a217ba987feb6d87f3e6b95919cadd8484efe69ecc364ba1731aaf173626c9
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 11, 2021
1cf9b35 doc: Add developer note on c_str() (Wladimir J. van der Laan)

Pull request description:

  Add a note when to use and when not to use `c_str()`.

ACKs for top commit:
  elichai:
    ACK 1cf9b35
  MarcoFalke:
    Looking nice ACK 1cf9b35

Tree-SHA512: 38cb5e54695782c23a82d03db214a8999b5bb52553f4fbe5322281686f42616981a217ba987feb6d87f3e6b95919cadd8484efe69ecc364ba1731aaf173626c9
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 15, 2021
1cf9b35 doc: Add developer note on c_str() (Wladimir J. van der Laan)

Pull request description:

  Add a note when to use and when not to use `c_str()`.

ACKs for top commit:
  elichai:
    ACK 1cf9b35
  MarcoFalke:
    Looking nice ACK 1cf9b35

Tree-SHA512: 38cb5e54695782c23a82d03db214a8999b5bb52553f4fbe5322281686f42616981a217ba987feb6d87f3e6b95919cadd8484efe69ecc364ba1731aaf173626c9
@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.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants