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

fix(ir): fix columns size for vector trace access #290

Merged
merged 1 commit into from
May 2, 2023
Merged

Conversation

tohrnii
Copy link
Contributor

@tohrnii tohrnii commented May 1, 2023

Addressing #289

@tohrnii tohrnii requested a review from grjte May 1, 2023 08:40
Copy link
Contributor

@grjte grjte left a comment

Choose a reason for hiding this comment

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

Nice catch, thank you! This looks correct to me. However, let's add a test for the issue that you're fixing before merging this.

@tohrnii
Copy link
Contributor Author

tohrnii commented May 1, 2023

let's add a test for the issue

I've modified the evaluators integration test.

@tohrnii tohrnii requested a review from grjte May 1, 2023 16:02
Copy link
Contributor

@grjte grjte left a comment

Choose a reason for hiding this comment

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

Thank you - one more request, sorry!

Comment on lines 19 to 24
enf is_unchanged([b])
enf are_unchanged([b, c[1], d[2]])
Copy link
Contributor

Choose a reason for hiding this comment

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

To really test both cases effectively, I think we should pass a column group to an evaluator function, e.g.

ev are_all_binary([c[3]]):
    enf is_binary([c[i]]) for i in 0..3

I know this is a stupid example, but the point is just to test passing in a SymbolType with AccessType::Default where the size is > 1.

I think this is my oversight from before the previous fix, since I think it was captured in another test elsewhere instead, but I'd appreciate it if you could update it

Copy link
Contributor Author

@tohrnii tohrnii May 1, 2023

Choose a reason for hiding this comment

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

Actually we don't yet support variables as indices for comprehensions in the parser. Maybe we should prioritize this issue since a lot of our constraints depend on this. Also, we haven't merged constraint comprehension for evaluators yet so I've added test for:

ev are_all_binary([c[3]]):
    enf c^2 = c for c in c

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok good point, thanks. That works! That's not the part I cared about for the test anyway

@tohrnii tohrnii requested a review from grjte May 1, 2023 16:57
Copy link
Contributor

@grjte grjte left a comment

Choose a reason for hiding this comment

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

Thank you, looks good!

@grjte grjte merged commit cb8fe39 into next May 2, 2023
@grjte grjte deleted the tohrnii-fix branch May 2, 2023 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants