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,test: fix JSON escaping in node-report #25626

Closed
wants to merge 3 commits into from

Conversation

lundibundi
Copy link
Member

Previously only simple escape sequences were handled
(i.e. \n, \t, r etc.). This commit adds escaping of other control
symbols in the range of 0x00 to 0x20.

Also, this replaces multiple find+replace calls with a single pass
replacer.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Previously only simple escape sequences were handled
(i.e. \n, \t, r etc.). This commit adds escaping of other control
symbols in the range of 0x00 to 0x20.

Also, this replaces multiple find+replace calls with a single pass
replacer.
@lundibundi lundibundi added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 21, 2019
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jan 21, 2019
@lundibundi
Copy link
Member Author

lundibundi commented Jan 21, 2019

src/node_report_utils.cc Outdated Show resolved Hide resolved
src/node_report_utils.cc Outdated Show resolved Hide resolved
test/cctest/test_report_util.cc Outdated Show resolved Hide resolved
@lundibundi
Copy link
Member Author

lundibundi commented Jan 21, 2019

@lundibundi lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 21, 2019
@lundibundi
Copy link
Member Author

lundibundi commented Jan 22, 2019

@addaleax addaleax added the report Issues and PRs related to process.report. label Jan 22, 2019
@danbev
Copy link
Contributor

danbev commented Jan 25, 2019

Landed in 80441c8.

@danbev danbev closed this Jan 25, 2019
danbev pushed a commit that referenced this pull request Jan 25, 2019
Previously only simple escape sequences were handled
(i.e. \n, \t, r etc.). This commit adds escaping of other control
symbols in the range of 0x00 to 0x20.

Also, this replaces multiple find+replace calls with a single pass
replacer.

PR-URL: #25626
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 28, 2019
Previously only simple escape sequences were handled
(i.e. \n, \t, r etc.). This commit adds escaping of other control
symbols in the range of 0x00 to 0x20.

Also, this replaces multiple find+replace calls with a single pass
replacer.

PR-URL: #25626
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@targos targos mentioned this pull request Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. report Issues and PRs related to process.report.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants