Skip to content
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

src: use .data() instead of .c_str() #11272

Closed
wants to merge 1 commit into from

Conversation

sam-github
Copy link
Contributor

Since C++11 onwards, .data() is required to be null terminated, and can
be used always instead of .c_str().

I mildly prefer c_str(), but if Node only supports C++11, this will convert to .data().

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

cf. #11006 (comment)

Since C++11 onwards, .data() is required to be null terminated, and can
be used always instead of .c_str().
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. debugger i18n-api Issues and PRs related to the i18n implementation. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. whatwg-url Issues and PRs related to the WHATWG URL implementation. inspector Issues and PRs related to the V8 inspector protocol labels Feb 9, 2017
@sam-github
Copy link
Contributor Author

/to @bnoordhuis

@sam-github
Copy link
Contributor Author

Though it appears to be segfaulting :-). One of the c_str() may have been required. I'll rebuild to see what's going on.

@addaleax
Copy link
Member

I mildly prefer c_str(), but if Node only supports C++11, this will convert to .data().

I personally prefer .c_str() for the cases where null termination actually matters and .data() when it doesn’t…

Anyway, if we want to do this bulk replacement, is there a way we can enforce it through the linter?

@sam-github
Copy link
Contributor Author

@bnoordhuis maybe we should go the other way, then, and always choose c_str()?

@bnoordhuis
Copy link
Member

I'm fine with c_str().

@sam-github sam-github closed this Feb 12, 2017
@sam-github sam-github deleted the use-data-not-cstr branch February 12, 2017 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. i18n-api Issues and PRs related to the i18n implementation. inspector Issues and PRs related to the V8 inspector protocol trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants