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

Seq #2346

Merged
merged 12 commits into from
Nov 12, 2024
Merged

Seq #2346

merged 12 commits into from
Nov 12, 2024

Conversation

eliascxstro
Copy link
Contributor

Configured various files and edited the parser to print out nodes in the format we talked about.
Biggest function implemented was string_path in program_counter.rs

@eliascxstro eliascxstro added the C: Interpreter / Cider Calyx Interpreter & Debugger label Nov 11, 2024
@eliascxstro eliascxstro self-assigned this Nov 11, 2024
Copy link
Collaborator

@EclecticGriffin EclecticGriffin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for the most part. Left a few notes here and there. Feel free to merge once those are taken care of. Also looks like there are some merge conflicts, so let me know if you need a hand resolving those

pub fn print_pc_string(&self) {
let current_nodes = self.pc.iter();
let ctx = &self.ctx.as_ref();
for node in current_nodes {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This & is unnecessary

Suggested change
for node in current_nodes {
let ctx = self.ctx.as_ref();

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea why this ended up on the wrong line

@@ -56,6 +56,67 @@ impl ControlPoint {
false
}
}

/// Returns a string showing the path from the root node to input node.
/// How to get context?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove /// How to get context? also specify that the path is displayed in the
minimal metadata path syntax

let path = SearchPath::find_path_from_root(self.control_node_idx, ctx);
let control_map = &ctx.primary.control;
let mut string_path = String::from("");
let mut count = -1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer new over from with an empty &str

Suggested change
let mut count = -1;
let mut string_path = String::new();

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my suggestion thing is just cursed, this also belongs on the line above

Comment on lines 99 to 102
if string_path.is_empty() {
string_path += ".";
continue;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hoist this out of the for loop and use in initialization of the string_path

Comment on lines +67 to +69
let mut count = -1;
let mut body = false;
let mut if_branches: HashMap<ControlIdx, String> = HashMap::new();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is fine and there's no need to change it here, but wanted
to flag the fact that if you zip the path with itself staggered by one
element, you can look at both the current node and the parent without needing
auxiliary structures. This can either be accomplished by processing the first
node specially or by appending a None value and mapping the parent field to an
option

@eliascxstro eliascxstro merged commit dd55f4a into main Nov 12, 2024
18 checks passed
@eliascxstro eliascxstro deleted the seq branch November 12, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Interpreter / Cider Calyx Interpreter & Debugger
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants