Skip to content

Compare ?T to T with ==/!= operators #5161

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

Closed
wants to merge 16 commits into from

Conversation

foobles
Copy link
Contributor

@foobles foobles commented Apr 25, 2020

Closes #1332.
Notes:

  • Does not currently work for comparing vector to ?vector. I have never used them, so I don't know how to implement this. It works otherwise.

@foobles foobles marked this pull request as ready for review May 2, 2020 18:48
@daurnimator daurnimator added the stage1 The process of building from source via WebAssembly and the C backend. label May 3, 2020
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Nice work! This is a non-trivial improvement to the core of the stage1 compiler.

I have an idea for how this can perhaps be done in a simpler way. Maybe you already tried it and ran into a problem. But here's the idea:

  • Leave ir_analyze_bin_op_cmp mostly the same, with only a check near the top for is_equality_cmp and detecting that we have a ?T compared with T situation. (or maybe split into two functions with only 1 that does the check)
  • In this case, call ir_analyze_test_non_null to check if the ?T is null. If the result is comptime known, then you can return a comptime bool right there, otherwise:
  • Use ir_build_cond_br_gen to do a runtime branch on whether the ?T is non-null. You can use ir_create_basic_block_gen to make the then, else, and end blocks and ir_set_cursor_at_end_gen to switch to each basic block.
  • In the null case, we have a comptime known comparison result; branch to the end block.
  • In the non-null case, call the ir_analyze_bin_op_cmp logic to do a comparison on T and T (no optionals here). branch to the end block.
  • In the end block, use ir_build_phi_gen to make a phi instruction, that selects the respective values from the then and else blocks.

I think you asked me about how to do this on IRC previously and I told you to create a new Gen instruction to do this logic. Sorry about that, sometimes it's hard to give good advice when I'm working on something else and don't have my head fully wrapped around the code.

Can you give this way of doing it a try? I think it will result in more "DRY" code with less room for bugs to creep up in the future.

comptime test_cmp_optional_non_optional();
}

fn test_cmp_optional_non_optional() void {
Copy link
Member

Choose a reason for hiding this comment

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

The test cases look good. Can you also add coverage for optional pointers (the motivating use case)?

@foobles
Copy link
Contributor Author

foobles commented May 5, 2020

@andrewrk Definitely, I didn't think that was possible. I'd be more than happy to use battle-tested combinators 👍

Also, is ?*T == *T just not tested as of now? Because it definitely already works without my change.

Additionally, should I just make a new PR? Since I basically have to revert everything here?

@foobles
Copy link
Contributor Author

foobles commented May 6, 2020

I think I will. See you in the next PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage1 The process of building from source via WebAssembly and the C backend.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow equality operator (== and !=) for ?T and T when equality operator is supported for T and T
3 participants