-
Notifications
You must be signed in to change notification settings - Fork 33
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.
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
Refactor Sed #5
base: master
Are you sure you want to change the base?
Refactor Sed #5
Conversation
Now using std::char for command selections and mut ref for globals
Hey @brookst, this looks like a beast of a PR... I love it! I'll set up some kind of CI so that we can start getting these tests running automatically. Let me know whenever you'd like me to start reviewing! |
Yeah, it seems like c2rust has a hard time replicating switch case statements. Which, after staring at them a little while, I can sympathise with. Its strategy seems to involve a lot of copy and paste so a few macros can strip out most of the repetition. The other top filesizes look suspicious:
Maybe a hit-list would be good for people to pick off utilities that need attention. Running the busybox tests on CI would be great! Having the tests ahead of time makes a nice change. |
I think you're absolutely right. And the relooper algorithm that c2rust uses to compile gotos also seems to produce quite a bit of overhead.
Good point, a hit list would definitely be useful! I'm torn between directing people to the "hairiest" parts of the codebase that could benefit the most from some serious rewrites vs directing people to the more approachable parts of the codebase that we have more hope of wholly rewriting. Another aspect here is that it may be worth simply bindgen-ing into some of these utilities in the future. Ie, is it worth it for the rustybox project maintain a Rust-fork of vi? Definitely open to discussion on this! |
Ok, so quick update on CI: I've set up a CircleCI workflow but unfortunately rustc exhausts the memory (4GB) offered on their free instances (https://app.circleci.com/jobs/github/samuela/rustybox/5). I've confirmed this on a local Docker setup. Not sure what can be done about rustc's memory usage... Sadly, I'm a PhD student so I can't afford to plop down $30/month on a side-project. Maybe we could find a sponsor for the project? |
GitHub Actions has 7GB of RAM, you may try that. |
Ok, we now have CI working thanks to @xfix! Next up, we'll need to start getting the busybox tests incorporated. |
The shape on sed looked pretty bad so I took a stab at improving things a bit.
I tested using the busybox tests (with
SKIP_KNOWN_BUGS=true
).The line count is down from 9284 to 1916 but there's still a bit of weird logic from switch/case statements hanging around. I'll take another look to see if I can get it down to around the same size as sed.c (1639 lines) but I thought it'd be good to post a PR in case anyone else started in this area.