ripgrep author here! This is a great review, thanks for doing it!
I'd like to respond to a few of the bad things pointed out. :P
The search code is indeed in a less than ideal state. I've mostly avoided refactoring it because I want to move it to its own separate crate. I've been steadily doing this for other things. Namely, ripgrep used to be a single main crate plus a small regex handling crate (grep), but now it's several: globset, grep, ignore, termcolor and wincolor. I'd like to roll the search code into the grep crate so that others can use it. Once that's done, ripgrep proper will be a pretty small, limited mostly to argv handling and output handling.
I do sometimes get overzealous with inline(always) and inline(never). Both are almost always a result of trying things while profiling, and then forgetting to remove them. If you look closely, most of them are in the core searching code where performance is quite important!
Finally, this code review was done while I was in the middle of moving more of ripgrep code out into the `ignore` and `termcolor` crates. The `ignore` crate does all the gitignore handling (which is quite tricky and is now being used by the tokei project: https://github.com/Aaronepower/tokei) and provides a parallel recursive directory iterator, which made ripgrep even faster! The `termcolor` crate handles cross platform coloring shenanigans, including Windows consoles and mintty. It wasn't fun: https://github.com/BurntSushi/ripgrep/issues/94#issuecomment... --- The author did a great job reviewing the previous solution I used for colors though, and was something I really wasn't proud of!
blog author here. Thanks for your feedback, I appreciate that! I'm gonna edit the post with it. And don't worry about your code... it's the best crate I've seen so far ;)
By the way, shouldn't msys_tty_on_handle be marked 'unsafe' too? I'm wondering if what happens I pass in some garbage as a handle...
I'm not sure, probably yes. It all depends on the winapi crate. If you can get an handle of a different "type" than the expected one, then it's garbage for GetFileInformationByHandleEx. Probably you can't do it without unsafe, so maybe it's fine.
I'd like to respond to a few of the bad things pointed out. :P
The search code is indeed in a less than ideal state. I've mostly avoided refactoring it because I want to move it to its own separate crate. I've been steadily doing this for other things. Namely, ripgrep used to be a single main crate plus a small regex handling crate (grep), but now it's several: globset, grep, ignore, termcolor and wincolor. I'd like to roll the search code into the grep crate so that others can use it. Once that's done, ripgrep proper will be a pretty small, limited mostly to argv handling and output handling.
I do sometimes get overzealous with inline(always) and inline(never). Both are almost always a result of trying things while profiling, and then forgetting to remove them. If you look closely, most of them are in the core searching code where performance is quite important!
Finally, this code review was done while I was in the middle of moving more of ripgrep code out into the `ignore` and `termcolor` crates. The `ignore` crate does all the gitignore handling (which is quite tricky and is now being used by the tokei project: https://github.com/Aaronepower/tokei) and provides a parallel recursive directory iterator, which made ripgrep even faster! The `termcolor` crate handles cross platform coloring shenanigans, including Windows consoles and mintty. It wasn't fun: https://github.com/BurntSushi/ripgrep/issues/94#issuecomment... --- The author did a great job reviewing the previous solution I used for colors though, and was something I really wasn't proud of!