Skip to content
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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Refactor Sed #5

wants to merge 5 commits into from

Conversation

brookst
Copy link

@brookst brookst commented Nov 16, 2019

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.

@samuela
Copy link
Owner

samuela commented Nov 16, 2019

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!

@brookst
Copy link
Author

brookst commented Nov 16, 2019

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:

lines bytes file
97893 4098170 ./editors/vi.rs
21590 838174 ./editors/awk.rs
15391 500307 ./shell/ash.rs
13089 477958 ./shell/hush.rs
8791 287802 ./miscutils/bc.rs
5473 164120 ./applets/applet_tables.rs
4409 168376 ./libbb/lineedit.rs
4045 165203 ./networking/udhcp/d6_dhcpc.rs
3852 147739 ./networking/udhcp/dhcpc.rs
3608 121713 ./util_linux/fdisk.rs

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.

@samuela
Copy link
Owner

samuela commented Nov 16, 2019

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.

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.

Maybe a hit-list would be good for people to pick off utilities that need attention.

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!

@samuela
Copy link
Owner

samuela commented Nov 16, 2019

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?

@KamilaBorowska
Copy link
Contributor

GitHub Actions has 7GB of RAM, you may try that.

@samuela
Copy link
Owner

samuela commented Nov 18, 2019

Ok, we now have CI working thanks to @xfix! Next up, we'll need to start getting the busybox tests incorporated.

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

Successfully merging this pull request may close these issues.

3 participants