Skip to content

Sema: Fix comparison with undefined 2#17990

Closed
wrongnull wants to merge 12 commits into
ziglang:masterfrom
wrongnull:fix-comprarison-with-undefined-2
Closed

Sema: Fix comparison with undefined 2#17990
wrongnull wants to merge 12 commits into
ziglang:masterfrom
wrongnull:fix-comprarison-with-undefined-2

Conversation

@wrongnull
Copy link
Copy Markdown
Contributor

Closes #17798
This is my second attempt to close this issue but with behavior test instead of compile-error one. I'm also not quite sure what should I do with this #17900 (comment) so @andrewrk please let me know what was wrong there.

Comment thread test/behavior/undefined.zig Outdated
Copy link
Copy Markdown
Member

@Vexu Vexu left a comment

Choose a reason for hiding this comment

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

I don't understand that review comment either.

Comment thread src/Sema.zig
Comment on lines -16512 to -16513
// For bools, we still check the other operand, because we can lower
// bool eq/neq more efficiently.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you look at git blame for this comment, it comes from 6e78c00.

The commit message there says that it was done for a particular desired codegen output. Since you are messing with this code and mentioned not understanding what this comment was talking about, it would be nice if the writeup for this patch demonstrated that this behavior did not regress. In other words, make sure x == true and x == false where x is a boolean are still lowered as a no-op or a not. There is no test coverage for this, so it has to be manually verified.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you look at git blame for this comment, it comes from 6e78c00.

The commit message there says that it was done for a particular desired codegen output. Since you are messing with this code and mentioned not understanding what this comment was talking about, it would be nice if the writeup for this patch demonstrated that this behavior did not regress. In other words, make sure x == true and x == false where x is a boolean are still lowered as a no-op or a not. There is no test coverage for this, so it has to be manually verified.

Just like the following?

wrongnull@DESKTOP-22AKPRT:~/projects/zig/build/stage3/bin$ ./zig version
0.12.0-dev.6066+ead7b6f5f
wrongnull@DESKTOP-22AKPRT:~/projects/zig/build/stage3/bin$ cat test.zig
pub export fn entry() u32 {
    const x = true;
    if (x == true) {return 0;} else {return 1;}
}
wrongnull@DESKTOP-22AKPRT:~/projects/zig/build/stage3/bin$ ./zig build-obj --verbose-air test.zig
# Begin Function AIR: test.entry:
# Total AIR+Liveness bytes: 299B
# AIR Instructions:         15 (135B)
# AIR Extra Data:           15 (60B)
# Liveness tomb_bits:       8B
# Liveness Extra Data:      0 (0B)
# Liveness special table:   0 (0B)
  %0!= save_err_return_trace_index()
  %2!= dbg_block_begin()
  %3!= dbg_stmt(2:5)
  %4!= dbg_var_val(@Air.Inst.Ref.bool_true, "x")
  %5!= dbg_stmt(3:9)
  %7!= dbg_block_begin()
  %9!= dbg_block_begin()
  %10!= dbg_stmt(3:21)
  %12!= dbg_block_end()
  %13!= dbg_block_end()
  %14!= dbg_block_end()
  %11!= ret(<u32, 0>)
# End Function AIR: test.entry```

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

x should be runtime known and the Air should not contain a cmp_eq.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

x should be runtime known and the Air should not contain a cmp_eq.

wrongnull@DESKTOP-22AKPRT:~/projects/zig/build/stage3/bin$ cat test.zig
pub export fn entry() u32 {
    var x = true;
    if (x == true) {return 0;} else {return 1;}
}
wrongnull@DESKTOP-22AKPRT:~/projects/zig/build/stage3/bin$ ./zig build-obj --verbose-air test.zig
# Begin Function AIR: test.entry:
# Total AIR+Liveness bytes: 479B
# AIR Instructions:         27 (243B)
# AIR Extra Data:           27 (108B)
# Liveness tomb_bits:       16B
# Liveness Extra Data:      2 (8B)
# Liveness special table:   1 (8B)
  %0!= save_err_return_trace_index()
  %2!= dbg_block_begin()
  %3!= dbg_stmt(2:5)
  %4 = alloc(*bool)
  %5!= store(%4, @Air.Inst.Ref.bool_true)
  %7!= dbg_var_ptr(%4, "x")
  %8!= dbg_stmt(3:9)
  %10 = load(bool, %4!)
  %26!= dbg_block_end()
  %25!= cond_br(%10!, {
    %11!= dbg_block_begin()
    %13!= dbg_block_begin()
    %14!= dbg_stmt(3:21)
    %16!= dbg_block_end()
    %17!= dbg_block_end()
    %15!= ret(<u32, 0>)
  }, {
    %18!= dbg_block_begin()
    %20!= dbg_block_begin()
    %21!= dbg_stmt(3:38)
    %23!= dbg_block_end()
    %24!= dbg_block_end()
    %22!= ret(<u32, 1>)
  })
# End Function AIR: test.entry

Comment thread test/cases/compile_errors/undefined_propagation_in_equality_operations.zig Outdated
@wrongnull wrongnull requested review from Vexu and andrewrk November 14, 2023 13:47
@andrewrk andrewrk changed the title Sema: Fix comprarison with undefined 2 Sema: Fix comparison with undefined 2 Jan 15, 2024
@andrewrk
Copy link
Copy Markdown
Member

andrewrk commented May 9, 2024

I'm sorry, I didn't review this in time, and now it has bitrotted. Furthermore, so many pull requests have stacked up that I can't keep up and I am therefore declaring Pull Request Bankruptcy and closing old PRs that now have conflicts with master branch.

If you want to reroll, you are by all means welcome to revisit this changeset with respect to the current state of master branch, and there's a decent chance your patch will be reviewed the second time around.

Either way, I'm closing this now, otherwise the PR queue will continue to grow indefinitely.

@andrewrk andrewrk closed this May 9, 2024
@wrongnull
Copy link
Copy Markdown
Contributor Author

I'm sorry, I didn't review this in time, and now it has bitrotted. Furthermore, so many pull requests have stacked up that I can't keep up and I am therefore declaring Pull Request Bankruptcy and closing old PRs that now have conflicts with master branch.

If you want to reroll, you are by all means welcome to revisit this changeset with respect to the current state of master branch, and there's a decent chance your patch will be reviewed the second time around.

Either way, I'm closing this now, otherwise the PR queue will continue to grow indefinitely.

Okay, don't worry. However, I'm very interested in the zig project, so I'll be sure to make changes accordingly to keep it fresh.

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.

missing comptime undefined propagation for equality operator with pointer, optional, error union, and integers

3 participants