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: zig 0.13.0-dev.46+3648d7df1 #276

Merged
merged 5 commits into from
May 5, 2024
Merged

fix: zig 0.13.0-dev.46+3648d7df1 #276

merged 5 commits into from
May 5, 2024

Conversation

WriteNaN
Copy link
Contributor

@WriteNaN WriteNaN commented May 1, 2024

Minor change to work with zig v13

@giann
Copy link
Collaborator

giann commented May 1, 2024

Thanks.

It would be easier to review without all the reformatting. Can you try and remove those?

@WriteNaN
Copy link
Contributor Author

WriteNaN commented May 1, 2024

sure, also tests were passing for me. i'll try debugging to see what caused the workflow to fail

@WriteNaN WriteNaN force-pushed the main branch 3 times, most recently from fa04b6c to 882741f Compare May 1, 2024 17:09
@WriteNaN
Copy link
Contributor Author

WriteNaN commented May 1, 2024

sorry for so many commits,

this commit has most of the changes which is very minor (switching from std.ComptimeStringMap).

that should be good now. lmk! 🙂

@giann
Copy link
Collaborator

giann commented May 1, 2024

No worries I'll squash all that :)

std.ComptimeStringMap -> std.StaticStringMap(T).initComptime()

semver check

switch should return a default value

update README

format to pass linting test
src/lib/buzz_os.zig Outdated Show resolved Hide resolved
src/FFI.zig Outdated
);
.{ "void", .{ .def_type = .Void } },
.{ "anyopaque", .{ .def_type = .Void } },
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

The reformatting is due to the last , being removed. Add them back and the diff will disappear.

@WriteNaN
Copy link
Contributor Author

WriteNaN commented May 3, 2024

that should be good?

@giann
Copy link
Collaborator

giann commented May 3, 2024

Almost: you should not have a else in the error switch. The new case should be the new zig error

@WriteNaN
Copy link
Contributor Author

WriteNaN commented May 3, 2024

oh oops! haha sorry

InvalidBatchScriptArg
@WriteNaN
Copy link
Contributor Author

WriteNaN commented May 3, 2024

that should be it! :D

@giann
Copy link
Collaborator

giann commented May 3, 2024

The error must also exists on the buzz side. You can add it under here:

https://github.com/buzz-language/buzz/blob/main/src/lib/errors.buzz#L3

The following means we're going to push on the stack the error errors.FileSystemError.InvalidBatchScriptArg.

ctx.vm.pushErrorEnum("errors.FileSystemError", @errorName(err))

Right now zig errors have their counterpart on the buzz side but I plan on simplifying those a little on the buzz side (#157)

@WriteNaN
Copy link
Contributor Author

WriteNaN commented May 3, 2024

The error must also exists on the buzz side. You can add it under here:

https://github.com/buzz-language/buzz/blob/main/src/lib/errors.buzz#L3

The following means we're going to push on the stack the error errors.FileSystemError.InvalidBatchScriptArg.

ctx.vm.pushErrorEnum("errors.FileSystemError", @errorName(err))

Right now zig errors have their counterpart on the buzz side but I plan on simplifying those a little on the buzz side (#157)

sorry I missed that, added now. Yes that is true. but I think the access to know what the error exactly is on buzz is also great.

@giann
Copy link
Collaborator

giann commented May 4, 2024

Huh. I'm not reproducing the CI error on my mac. Seems like the mir building step is doing it on a different cpu from the following steps.

@giann giann mentioned this pull request May 5, 2024
@giann
Copy link
Collaborator

giann commented May 5, 2024

Merging anyway, i'll temporarily disable the macos test if this persists

@giann giann closed this May 5, 2024
@giann giann reopened this May 5, 2024
@giann giann merged commit 8f1c900 into buzz-language:main May 5, 2024
4 of 6 checks passed
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