-
Notifications
You must be signed in to change notification settings - Fork 38.8k
doc: Add developer note on c_str() #17281
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.
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
Conversation
ryanofsky
left a comment
There was a problem hiding this 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.
|
ACK |
… 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()`.
217e4a5 to
1cf9b35
Compare
|
Addressed comments and squashed. |
|
ACK 1cf9b35 |
|
Looking nice ACK 1cf9b35 |
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
… 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
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
…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
… 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
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
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
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
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
Add a note when to use and when not to use
c_str().