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

add #[must_use] new_call function to show warning #30

Merged
merged 5 commits into from
Mar 21, 2024

Conversation

mubarak23
Copy link

No description provided.

Copy link

@antoyo antoyo left a comment

Choose a reason for hiding this comment

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

A small nitpick.

@@ -107,6 +107,7 @@ fn codegen<'a, 'ctx>(ops: &[Op], context: &'a gccjit::Context<'ctx>) -> bool {
let mut current_block = brainf_main.new_block("entry_block");
// now we have to zero out the giant buffer we just allocated on the stack.
let zero_access = context.new_array_access(None, array.to_rvalue(), context.new_rvalue_zero(int_ty));
// a call to context.new_call will give us warning
Copy link

Choose a reason for hiding this comment

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

I would only keep this comment, so please remove the others.

And please edit the comment to something like this:

Suggested change
// a call to context.new_call will give us warning
// A function call that is done for its side effects must be sent to add_eval.

Copy link
Author

Choose a reason for hiding this comment

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

Alright

@mubarak23
Copy link
Author

@antoyo these changes has been push as requested

Copy link

@antoyo antoyo left a comment

Choose a reason for hiding this comment

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

Another nitpick :) .
I can merge after you fix it.
Thanks a lot!

@@ -172,11 +173,13 @@ fn codegen<'a, 'ctx>(ops: &[Op], context: &'a gccjit::Context<'ctx>) -> bool {
}
Op::Input => {
let access = context.new_array_access(None, array.to_rvalue(), memory_ptr.to_rvalue());

Copy link

Choose a reason for hiding this comment

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

Please remove this new line and the others below.

Copy link
Author

Choose a reason for hiding this comment

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

Alright

@mubarak23
Copy link
Author

@antoyo new line remove

@antoyo
Copy link

antoyo commented Mar 21, 2024

@antoyo new line remove

There are a few more that are left here and there, so please remove them as well.

@mubarak23
Copy link
Author

oh, really,

@mubarak23
Copy link
Author

mubarak23 commented Mar 21, 2024

@antoyo space remove

@antoyo antoyo merged commit 55b63b4 into rust-lang:master Mar 21, 2024
1 check passed
@antoyo
Copy link

antoyo commented Mar 21, 2024

Thanks for your contribution.

@antoyo
Copy link

antoyo commented Mar 21, 2024

@mubarak23: Also, please do not ping me as I get a notification from GitHub so I already know you pushed your changes :) .
I'll review them when I have time (still work hours here for me).

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