Add a twiggy garbage command. Fixes issue #48.
#50
Conversation
analyze/analyze.rs
Outdated
| /// Find items that are not transitively referenced by any exports or public functions. | ||
| pub fn garbage( | ||
| _items: &ir::Items, | ||
| _opts: &opt::Garbage, |
There was a problem hiding this comment.
Variables with a leading underscore are usually reserved for unused variables. Stylistically, it is a little weird for these parameters to be used despite the leading underscore. These should be changed to plain items and opts.
There was a problem hiding this comment.
Oh shoot! I meant to change those, I had placed the garbage function declaration in with an unimplemented macro when I was working on the plumbing elsewhere. Will do!
analyze/analyze.rs
Outdated
| _items: &ir::Items, | ||
| _opts: &opt::Garbage, | ||
| ) -> Result<Box<traits::Emit>, traits::Error> { | ||
| fn get_reachable_items(_items: &ir::Items) -> BTreeSet<ir::Id> { |
analyze/analyze.rs
Outdated
| #[derive(Debug)] | ||
| struct Garbage { | ||
| items: Vec<ir::Id>, | ||
| opts: opt::Garbage, |
There was a problem hiding this comment.
Is the opts field ever used? If not, then we should remove it.
twiggy/tests/expectations/garbage
Outdated
| @@ -0,0 +1,11 @@ | |||
| Bytes │ Size % │ Item | |||
There was a problem hiding this comment.
Can you check in the WAT source used to generate garbage.wasm too? That way we can easily make changes if we need to update it in the future.
twiggy/tests/expectations/garbage
Outdated
| Bytes │ Size % │ Item | ||
| ───────┼────────┼──────── | ||
| 11 ┊ 13.10% ┊ code[2] | ||
| 8 ┊ 9.52% ┊ code[1] |
There was a problem hiding this comment.
We should probably also build garbage.wasm with the --debug-names flag, so we get function names instead of code[N].
README.md
Outdated
| transitively referenced by any exports or public functions. | ||
|
|
||
| ``` | ||
| AWOO : Place output example here. |
analyze/Cargo.toml
Outdated
| @@ -15,3 +15,4 @@ path = "./analyze.rs" | |||
| twiggy-ir = { version = "0.1.0", path = "../ir" } | |||
| twiggy-opt = { version = "0.1.0", path = "../opt", default-features = false } | |||
| twiggy-traits = { version = "0.1.0", path = "../traits" } | |||
| petgraph = "0.4.12" | |||
There was a problem hiding this comment.
Right now we depend on an older point release in ir/Cargo.toml:
ir/Cargo.toml:17:petgraph = "0.4.11"
Can you update that so that we have the same dependency? Thanks!
analyze/analyze.rs
Outdated
| let mut table = Table::with_header(vec
|
Thanks for the review! I can take care of these changes tonight :) |
* Remove leading underscores in variables in the `garbage` function. * Check in the WAT source for the used for the garbage test fixture. * Compile the test fixture using wat2wasm's `--debug-names` flag. * Add missing example output to `README.md`. * Adjusted `petgraph` dependency to use same version used elsewhere in project. * Update name of item column when printing to stdout. * Remove extraneous `opts` member from the `Garbage` struct.
|
I added the patches from your review @fitzgen :) I left this as two separate commits so you can review it faster, but I can rebase these into a single commit if you would like! |
This fixes #48, and adds a
garbagesubcommand. This includes a test fixture in the form of a wasm module, the source of which can be found here.Let me know if there are any changes or extra options we should add, I'd be happy to do so. Thanks for the mentoring instructions on this issue, had a lot of fun getting this done! Twiggy is a really neat project :D