Implement unescape helper and style fixes.

digitalbazaar / rdf-canonize-native

A native implementation of the RDF Dataset Normalization Algorithm for Node.js.

Other

6 stars 0 forks source link

Implement unescape helper and style fixes. #2

Closed mattcollier closed 5 years ago

mattcollier commented 5 years ago

Although the test suite is passing, I don't know if this C++ code is doing the right thing, or if it's implemented properly, but I figured I'd get the ball rolling.

Just commented regex code so comparison would be easier, this stuff should be removed.

I found the benchmarks too variable to be useful, see: https://gist.github.com/mattcollier/33613cef6565c22cb78e84e20e34d260

Lifted the unescape code from here: https://stackoverflow.com/a/5612287

mattcollier commented 5 years ago

Added a benchmark that shows a 100% performance increase with the unescape implementation. See results: https://gist.github.com/mattcollier/d01faf4e2f2be59f9d8796523e7f3a50

dlongley commented 5 years ago

@davidlehn, pulling this in would yield a very significant performance increase -- can you see if we have proper testing for it and add a test or two if not?

davidlehn commented 5 years ago

I'm not sure what extra testing is needed. I'm guessing it'd be some crazy escaping edge case. Would be in the main test suite if so. Any thoughts on oddball patterns that could cause trouble?
If we're not using regexes then can clean up that file: remove regex include, remove regexes, remove commented out code, move iostream header up, remove other unused commented old things.
I think that regex call was the only reason we needed all that goofy travis compiler update code. Would be nice to not need that!

mattcollier commented 5 years ago

@davidlehn please make whatever changes are necessary here and wrap it up.

That commented about test suite stuff was not running properly, it doesn't seem to be a thing in the current test suites if you download a new copy of it.