adding extra dominators #19
Conversation
formatting
There was a problem hiding this comment.
This looks good @sendilkumarn !
I have a few comments that should be addressed before we merge this, see comments inline below.
Thanks!
analyze/analyze.rs
Outdated
| @@ -196,6 +196,7 @@ pub fn top(items: &mut ir::Items, opts: &opt::Top) -> Result<Box<traits::Emit>, | |||
|
|
|||
| struct DominatorTree { | |||
| tree: BTreeMap<ir::Id, Vec<ir::Id>>, | |||
| dominators: opt::Dominators, | |||
There was a problem hiding this comment.
Nitpick: let's rename this field to opts
analyze/analyze.rs
Outdated
| id: ir::Id, | ||
| ) { | ||
| assert_eq!(id == items.meta_root(), depth == 0); | ||
|
|
||
| if depth > 0 { | ||
| let item = &items[id]; | ||
| if dominators.max_row != 0 && depth != dominators.max_depth { |
There was a problem hiding this comment.
Rather than indenting the whole function inside this if, let's:
- add a new parameter,
row: &mut usize - add an early return at the start of this function
if *row == opts.max_rows || depth > opts.max_depth { return; } - whenever we actually add a row to the table,
*row += 1; - finally, when we first call
recursive_add_rows, pass&mut 0for therowsparameter
These tweaks should make this functionality a little less invasive
opt/opt.rs
Outdated
|
|
||
| #[structopt(short = "d", default_value = "10")] pub max_depth: usize, | ||
|
|
||
| #[structopt(short = "r", default_value = "10")] pub max_row: usize, |
There was a problem hiding this comment.
NItpick: max_rows instead of `max_row.
Let's also add doc comments to each new field, since this will turn into the CLI help text:
The maximum depth to print the dominators tree.
The maximum number of rows, regardless of depth in the tree, to display.
Let's also make these Option<usize> rather than providing a default value.
analyze/analyze.rs
Outdated
| if dominators.max_row != 0 && depth != dominators.max_depth { | ||
| if depth > 0 { | ||
| let item = &items[id]; | ||
| if let Some(max_rows) = opts.max_rows { |
There was a problem hiding this comment.
Do we have a better syntax here?
This RFC should be handy
There was a problem hiding this comment.
Yeah that would be nice, but it is fine as-is, and better than it was before with the indentation of everything. Thanks again!
fixes #13