lib: converge part2: logic to create value flow graphs and finding the value dominator#9543
lib: converge part2: logic to create value flow graphs and finding the value dominator#9543drieber wants to merge 1 commit into
Conversation
scott2000
left a comment
scott2000
left a comment
There was a problem hiding this comment.
Looks good to me overall
|
|
||
| // Below is the implementation of value flow graphs and dominator value finding | ||
| // for FlowGraphs. | ||
| impl<N> FlowGraph<N> |
There was a problem hiding this comment.
nit: it might be easier to read if these methods are moved into the existing impl block for FlowGraph
There was a problem hiding this comment.
I combined the two impl<N> FlowGraph<N> blocks into one by moving the new method into this block. Let me know if you prefer I put everything into the existing impl block (as you had suggested). At first I thought it is better later in the file, but I do not have a strong opinion about this.
There was a problem hiding this comment.
Normally I think it's nice to keep the impl block near the type declaration to make it easier to find, but I don't have a strong opinion. Another option would be to move the FlowGraph type later in the file as well.
| .map_err(|e| FindDominatorValueError::ValueFnError(e))?; | ||
|
|
||
| // num_final_values is the number of *distinct* final values. | ||
| let final_values = value_cache.get_values(); |
There was a problem hiding this comment.
If I'm understanding correctly, it looks like this assumes that the value_cache is initially empty (or it only contains final nodes) before this function is called. Maybe it would be good to either document this assumption, or construct the ValueCache directly inside this function to ensure it's initially empty?
Or if find_dominator_value_with_value_cache isn't planned to be called directly, maybe it could be inlined into find_dominator_value instead.
There was a problem hiding this comment.
I don't assume value_cache is empty. The jj-converge library logic (in PR/9345) calls find_dominator_value_with_value_cache. find_dominator_value is only used in tests inside this file, but I made it public because it is a simpler API that could in theory be useful.
I improved the comment on top of find_dominator_value_with_value_cache a bit, but the comment does not say whether value_cache should be empty or not. I think there is no need to. But if you prefer I could add "Note that value_cache may or may not be empty."
There was a problem hiding this comment.
I'm still a bit confused. Based on the naming, it seems to me like final_values is supposed to contain only the values corresponding to final_nodes. However, if value_cache contains additional nodes beyond final_nodes, then final_values will also contain additional values I believe (but maybe I misunderstood).
If this behavior is intentional, maybe all_cached_values would be a better name than final_values, since that makes it more clear that they don't necessarily correspond to final_nodes?
There was a problem hiding this comment.
I see what you are saying. If the value_cache is not empty then the cardinality comparisons I am relying on may not make sense. That is a valid point. Probably the best way forward is to remove those optimizations from this PR, that can be added later if necessary. WDYT?
drieber
force-pushed
the
push-zymvuwzoloqs
branch
from
ffedc63 to
d64ecaa
Compare
3 participants
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)how it works, how it's organized), including any code drafted by an LLM.
an eye towards deleting anything that is irrelevant, clarifying anything
that is confusing, and adding details that are relevant. This includes,
for example, commit descriptions, PR descriptions, and code comments.