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.

Sign up for GitHub

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

Jump to bottom

parser: improve string parsing #66

Closed
wants to merge 1 commit into from
Closed

parser: improve string parsing #66

wants to merge 1 commit into from

Conversation

belochub
Copy link
Member

  • Eliminate unnecessary assignments.
  • Encode control characters in place to avoid copying and memory allocations.

@belochub belochub requested a review from aqrln February 10, 2017 16:03
aqrln
aqrln reviewed Feb 10, 2017
View reviewed changes
src/jsrs_parser.cc Outdated
const char* str,
size_t* res_len,
size_t* size,
char* write_to);
Copy link
Member

Choose a reason for hiding this comment

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

Please align the parameter to match the surrounding style.

aqrln
aqrln reviewed Feb 10, 2017
View reviewed changes
src/jsrs_parser.cc
char* symb =
GetControlChar(isolate, begin + ++i, &out_offset, &in_offset);
if (!symb) {
bool ok = GetControlChar(isolate, begin + ++i, &out_offset, &in_offset,
Copy link
Member

Choose a reason for hiding this comment

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

So that doesn't need copying at all, even to a stack-allocated buffer. Good catch!

aqrln
aqrln reviewed Feb 10, 2017
View reviewed changes
src/jsrs_parser.cc
if (!symb) {
bool ok = GetControlChar(isolate, begin + ++i, &out_offset, &in_offset,
result + res_index);
if (!ok) {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making it one-line?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think, that way it is more understandable than just

if (!GetControlChar(...)) {

Compiler should optimize it anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, for some reason it seemed to me that there's one short statement inside the if body, so I proposed to eliminate braces.

aqrln
aqrln reviewed Feb 10, 2017
View reviewed changes
src/jsrs_parser.cc Outdated
const char* str,
size_t* res_len,
size_t* size,
char* write_to) {
Copy link
Member

Choose a reason for hiding this comment

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

The same alignment issue as above.

aqrln
aqrln reviewed Feb 10, 2017
View reviewed changes
src/jsrs_parser.cc
break;
}
case 'v': {
*result = '\v';
*write_to = '\v';
break;
}
case '0': {
Copy link
Member

Choose a reason for hiding this comment

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

(Not related to this PR)
Do we really need '\0' as a special case here? Either we support any octal sequences, or we don't support them at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I guess we don't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will make a new PR for this.

@belochub
Copy link
Member Author

belochub commented Feb 10, 2017 •
edited
Loading

Added fix for possible memory leak in string parsing.

@belochub
parser: improve string parsing
362e73c 
* Eliminate unnecessary assignments.
* Encode control characters in place to avoid copying and memory
  allocations.
* Fix possible memory leak.
@belochub
Copy link
Member Author

@aqrln updated with alignment fixes.

aqrln
aqrln approved these changes Feb 10, 2017
View reviewed changes
Copy link
Member

@aqrln aqrln left a comment

Choose a reason for hiding this comment

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

LGTM

belochub added a commit that referenced this pull request Feb 10, 2017
@belochub
parser: improve string parsing
779965d 
* Eliminate unnecessary assignments.
* Encode control characters in place to avoid copying and memory
  allocations.
* Fix possible memory leak.

PR-URL: #66
@belochub
Copy link
Member Author

Landed in 779965d.

@belochub belochub closed this Feb 10, 2017
@belochub belochub deleted the string-parsing-improvements branch February 10, 2017 17:02
aqrln pushed a commit that referenced this pull request Feb 20, 2017
@belochub @aqrln
parser: improve string parsing
acb4146 
* Eliminate unnecessary assignments.
* Encode control characters in place to avoid copying and memory
  allocations.
* Fix possible memory leak.

PR-URL: #66
@aqrln aqrln mentioned this pull request Feb 20, 2017
Merged
aqrln pushed a commit that referenced this pull request Feb 20, 2017
@belochub @aqrln
parser: improve string parsing
e09ecba 
* Eliminate unnecessary assignments.
* Encode control characters in place to avoid copying and memory
  allocations.
* Fix possible memory leak.

PR-URL: #66
aqrln pushed a commit that referenced this pull request Apr 2, 2017
@belochub @aqrln
parser: improve string parsing
7d83901 
* Eliminate unnecessary assignments.
* Encode control characters in place to avoid copying and memory
  allocations.
* Fix possible memory leak.

PR-URL: #66
aqrln pushed a commit that referenced this pull request Apr 2, 2017
@belochub @aqrln
parser: improve string parsing
6a61a18 
* Eliminate unnecessary assignments.
* Encode control characters in place to avoid copying and memory
  allocations.
* Fix possible memory leak.

PR-URL: #66
aqrln pushed a commit that referenced this pull request Apr 3, 2017
@belochub @aqrln
parser: improve string parsing
33b64f7 
* Eliminate unnecessary assignments.
* Encode control characters in place to avoid copying and memory
  allocations.
* Fix possible memory leak.

PR-URL: #66
@aqrln aqrln mentioned this pull request Apr 3, 2017
Merged
belochub added a commit that referenced this pull request Jan 22, 2018
@belochub
parser: improve string parsing
6521f7c 
* Eliminate unnecessary assignments.
* Encode control characters in place to avoid copying and memory
  allocations.
* Fix possible memory leak.

PR-URL: #66
belochub added a commit that referenced this pull request Jan 22, 2018
@belochub
parser: improve string parsing
a1d8602 
* Eliminate unnecessary assignments.
* Encode control characters in place to avoid copying and memory
  allocations.
* Fix possible memory leak.

PR-URL: #66
@belochub belochub mentioned this pull request Jan 22, 2018
Merged
belochub added a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
@belochub
parser: improve string parsing
309aede 
* Eliminate unnecessary assignments.
* Encode control characters in place to avoid copying and memory
  allocations.
* Fix possible memory leak.

PR-URL: metarhia/jstp#66
belochub added a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
@belochub
parser: improve string parsing
08db614 
* Eliminate unnecessary assignments.
* Encode control characters in place to avoid copying and memory
  allocations.
* Fix possible memory leak.

PR-URL: metarhia/jstp#66
belochub added a commit to metarhia/mdsf that referenced this pull request Jul 21, 2018
@belochub
parser: improve string parsing
9016b29 
* Eliminate unnecessary assignments.
* Encode control characters in place to avoid copying and memory
  allocations.
* Fix possible memory leak.

PR-URL: metarhia/jstp#66
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aqrln aqrln aqrln approved these changes

Assignees
No one assigned
Labels
optimization parser semver-patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@belochub @aqrln