3 ms·

I appreciate the effort, but as a C++ guy, code like this scares the hell out of me. Just a quick glance and I already have the impression that the code is full

by pubby 9y ago

I appreciate the effort, but as a C++ guy, code like this scares the hell out of me. Just a quick glance and I already have the impression that the code is full of memory leaks, UB, and bad code reuse. Plus, it has really bizarre stuff like creating its own bool type and no use of the standard library besides iostreams (lol iostreams). I also do not believe the benchmarks to be meaningful.

guitarbill 9y ago
Well, unless you have a Mac with the user "btwael", the benchmark might prove tough to use as it's hardcoded to "/Users/btwael/Downloads/longtextfile.txt". Sadly, this file isn't included in the repo. With 2280 lines, a unit test or two would make me way less apprehensive.
msim 9y ago
This. The UTF-8 decoding is broken, and I didn't look at anything beyond that, resulting in a heap buffer overflow with untrusted input. And here is a summary of the warnings I get when compiling with "-Weverything": 26 [-Wcast-align] 20 [-Wcast-qual] 6 [-Wconditional-uninitialized] 24 [-Wdollar-in-identifier-extension] 172 [-Wold-style-cast] 2 [-Wshorten-64-to-32] 3 [-Wsign-compare] 25 [-Wsign-conversion] 3 [-Wunused-parameter] 72 [-Wzero-as-null-pointer-constant] Please don't use this code.
- bananapeel 9y ago
  Actually, the UTF-8 decoding is broken even with proper input. This https://github.com/btwael/SuperString/blob/master/src/SuperString.cc#L2015 https://github.com/btwael/SuperString/blob/master/src/SuperS... will make `pointer` and `i` be out of sync, so everything after the first multibyte sequence is a coin toss. To the OP: you should use theses tests to validate your implementation https://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-test.txt https://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-test.txt (caveat: 5- and 6-byte sequences are no longer valid UTF-8 and should be rejected).

by pubby 9y ago

guitarbill 9y ago

Well, unless you have a Mac with the user "btwael", the benchmark might prove tough to use as it's hardcoded to "/Users/btwael/Downloads/longtextfile.txt". Sadly, this file isn't included in the repo. With 2280 lines, a unit test or two would make me way less apprehensive.

msim 9y ago

This. The UTF-8 decoding is broken, and I didn't look at anything beyond that, resulting in a heap buffer overflow with untrusted input. And here is a summary of the warnings I get when compiling with "-Weverything": 26 [-Wcast-align] 20 [-Wcast-qual] 6 [-Wconditional-uninitialized] 24 [-Wdollar-in-identifier-extension] 172 [-Wold-style-cast] 2 [-Wshorten-64-to-32] 3 [-Wsign-compare] 25 [-Wsign-conversion] 3 [-Wunused-parameter] 72 [-Wzero-as-null-pointer-constant] Please don't use this code.

bananapeel 9y ago
Actually, the UTF-8 decoding is broken even with proper input. This https://github.com/btwael/SuperString/blob/master/src/SuperString.cc#L2015 https://github.com/btwael/SuperString/blob/master/src/SuperS... will make `pointer` and `i` be out of sync, so everything after the first multibyte sequence is a coin toss. To the OP: you should use theses tests to validate your implementation https://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-test.txt https://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-test.txt (caveat: 5- and 6-byte sequences are no longer valid UTF-8 and should be rejected).