Conversation
This filters out all `lang_start` items (functions, closures, drop glue etc.). It only removes the items if they are `std::rt::lang_start` or are exclusively reachable by those functions. That means `black_box` is removed if it is not called otherwise in the program, but if it is then it is not removed. There is a couple of stages to get the data together in the right form so that only the items desired to be removed are.
dkcumming
requested review from
jberthold,
mariaKt,
palinatolmach and
tothtamas28
cds-amal
left a comment
cds-amal
left a comment
There was a problem hiding this comment.
This is really nice @dkcumming, I love the story in your commits. I feel strongly about including a test to surface regressions, as I expect there are many more features to come.
There are no integration or unit tests for the SKIP_LANG_START behavior. The existing .smir.json.expected files all contain lang_start items, so there's good data to work from. At minimum, an integration test that runs with SKIP_LANG_START=1 and verifies the output lacks lang_start entries would give confidence this doesn't regress.
| let excluded: HashSet<String> = if skip_lang_start() { | ||
| let excluded = compute_lang_start_exclusions(&self.items, &ctx); | ||
| self.items.retain(|i| !excluded.contains(&i.symbol_name)); | ||
| excluded | ||
| } else { | ||
| HashSet::new() | ||
| }; |
There was a problem hiding this comment.
This is a useful filter op. I recommend moving it; could be a free fn in the mk_graph mod.
| let Some(callee_name) = ctx.resolve_call_target(func) else { | ||
| continue; | ||
| }; | ||
| if !is_unqualified(&callee_name) { | ||
| if !is_unqualified(&callee_name) || excluded.contains(&callee_name) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Want to point out that we have to thread excluded into this function to filter a target. Seems. like a smell -- Not asking for a change :)
Right now the flow is:
- Build ctx: populates ctx.functions with everything
- Compute excluded
- self.items.retain(...) to remove excluded items
- Thread excluded through every render function to check at call-edge time
If instead, step 3 also did:
ctx.functions.retain(|_, name| !excluded.contains(name));then ctx.resolve_call_target() would return None for excluded functions, and the existing early-return in D2 handles it already:
let Some(callee_name) = ctx.resolve_call_target(func) else {
continue; // already skips when resolve returns None
};
// no need for: || excluded.contains(&callee_name)
if !is_unqualified(&callee_name) {
continue;
}That would also eliminate the need to compute (locally) and thread &excluded entirely. NOTE: This would likely need the dot renderer to be factored.
| let excluded: HashSet<String> = if skip_lang_start() { | ||
| let excluded = compute_lang_start_exclusions(&self.items, &ctx); | ||
| self.items.retain(|i| !excluded.contains(&i.symbol_name)); | ||
| excluded | ||
| } else { | ||
| HashSet::new() | ||
| }; |
There was a problem hiding this comment.
Aha! Good ole copy-and-paste inheritance (or composition in this case). This supports the filter refaction.
2 participants
This PR allows us to set a
SKIP_LANG_START=1env var to remove thestd::rt::lang_startitems from the graph output. Perhaps sometimes they are useful, but I have not ever needed to look at them and would prefer them to be removed from the graph.I had a bit of trouble getting the correct items for the BFS and I used claude to help for full disclosure, however I have vetted the code and agree with it. Also the output has been tested and it does remove the items correctly. I will post before and after of a contrived program that contains
black_boxto show that the algorithm will only remove items exclusively downstream ofstd::rt::lang_start.Before
After