Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Add some convenience APIs to OnigString #64

Merged
merged 3 commits into from
Apr 19, 2017

Conversation

maxbrunsfeld
Copy link
Contributor

This adds three APIs to the OnigString class:

  • .length
  • .toString
  • .substring

We pass the lines to tokenize around to a ton of different methods in first-mate. This way, we can replace the string with an OnigString without having to also pass the original string around.

I've also converted all the CoffeeScript to JS.

@maxbrunsfeld maxbrunsfeld merged commit a485076 into master Apr 19, 2017
@maxbrunsfeld maxbrunsfeld deleted the mb-onig-string-properties branch April 19, 2017 22:43
return;
}

OnigString* string = new OnigString(content);
Copy link
Contributor

@thomasjo thomasjo Apr 20, 2017

Choose a reason for hiding this comment

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

Looks like this might be leaking, unless there's some magic going on in node::ObjectWrap? 🚰
Regardless, it can be modernized to using smart pointers to guard against leaks;

std::shared_ptr<OnigString> string = std::make_shared<OnigString>(content);

Personally I also would have used the auto keyword here, but we don't seem to be using that in any of our C++ projects, hence the full type. And note that if the Wrap function does perform magic, smart pointers might not work as intended 🎱

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think the ObjectWrap class causes delete to be called in the Javascript object's finalizer.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants