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

Update gen.zig to produce just enums #242

Closed

Conversation

taylorh140
Copy link
Contributor

Changed the gen.zig to produce just enums instead of packed unions.

@taylorh140
Copy link
Contributor Author

as per @ikskuh's statement. that built-ins remove the need for the packed unions.

Copy link
Contributor

@mattnite mattnite left a comment

Choose a reason for hiding this comment

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

You'll have to change some of the unit tests in addition to the comments I've made thus far.

tools/regz/src/gen.zig Outdated Show resolved Hide resolved
tools/regz/src/gen.zig Show resolved Hide resolved
tools/regz/src/gen.zig Show resolved Hide resolved
@taylorh140
Copy link
Contributor Author

I'm not quite sure where the test are located... yet. Ill find them and get this polished.

@mattnite
Copy link
Contributor

mattnite commented Oct 3, 2024

@taylorh140 tests are at the bottom of the file

@taylorh140
Copy link
Contributor Author

Ok, I think I have the changes you requested. I'm not sure why the build is failing though.

I think I have the test cases aligned but. However i can seem to build zig test. I'll have to see if I added that.

@taylorh140
Copy link
Contributor Author

taylorh140 commented Oct 4, 2024

It doesnt look like I added it:
regz
zig build test gives:
image

@taylorh140 taylorh140 requested a review from mattnite October 18, 2024 19:29
@mattnite
Copy link
Contributor

@taylorh140 check the generated code, we're failing AST checks. Could be something like a missing curly brace

@taylorh140
Copy link
Contributor Author

@mattnite Hey it looks like the AST problem is better, I missed removing a closing bracket in the extended cases.

I think that the next thing is maybe updating how some of the items use the packed union as it looks like it's causing some issues. I think i wouldn't mind hunting these down if you don't thing it would be too much of a scope creep for the PR.

@mattnite
Copy link
Contributor

@taylorh140 Yup, a low level API has been changed so now all uses of it need to be updated. I care more about commit atomicity than perceived patch complexity, so to me it's not even scope creep, it's an unfinished part of the task you're trying to do.

Fixing mistakes on number of items included, in print and removed comments.
Redid test cases.
Unmatched closing bracket.
@mattnite
Copy link
Contributor

Closing because #328 implements this now that the regz rewrite is complete.

@mattnite mattnite closed this Dec 22, 2024
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