Skip to content

less undefined #4492

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 19 commits into from
Closed

less undefined #4492

wants to merge 19 commits into from

Conversation

CGQAQ
Copy link
Contributor

@CGQAQ CGQAQ commented Sep 5, 2023

What does this PR do?

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

This PR includes:

.? should be optimized away in non-safe mode, so these changes should not impact performance, just make use of these nullable pointers more explicit

Closes: #4426

@CGQAQ CGQAQ changed the title less undefind less undefined Sep 5, 2023
@CGQAQ CGQAQ marked this pull request as ready for review September 6, 2023 02:23
Comment on lines +537 to 548
/// This is wrong! Avoid using this!
/// Any use of this should be replaced with the type itself to the optional type.
/// The undefind is unspecified value, cant be compared to anything include itself.
/// In Debug mode,
/// Zig writes 0xaa bytes to undefined memory.
/// This is to catch bugs early, and to help detect use of undefined memory in a debugger.
/// However, this behavior is only an implementation feature, not a language semantic,
/// so it is not guaranteed to be *observable* to code.
/// ref: https://ziglang.org/documentation/master/#undefined
pub fn assertDefined(val: anytype) void {
if (comptime !Environment.allow_assert) return;
const Type = @TypeOf(val);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should use the optional type when we need this

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 6, 2023

@dylan-conway Please take a look. Sorry for so many changes in a single PR, but they're pretty straightforward.

@CGQAQ

This comment was marked as resolved.

@Jarred-Sumner
Copy link
Collaborator

I think this is an interesting start but we shouldn't replace all usages of undefined with optional. We can do this more gradually. It'll be too difficult to confidently merge in this current state. When they're not pointers, marking things as optional can increase the size and alignment of structs which can in of itself cause new bugs

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 6, 2023

I think this is an interesting start but we shouldn't replace all usages of undefined with optional. We can do this more gradually. It'll be too difficult to confidently merge in this current state. When they're not pointers, marking things as optional can increase the size and alignment of structs which can in of itself cause new bugs

This PR only changes ptr fields in the struct that default initialized to undefined to optional ptr, as an optional pointer is guaranteed to be the same size as a pointer. The null of the optional is guaranteed to be address 0, and only few non ptr type changes, so it should not cause any new bugs

@CGQAQ CGQAQ closed this Sep 27, 2023
@CGQAQ CGQAQ deleted the less-undefined branch September 27, 2023 00:43
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.

Audit every usage of the undefined keyword in our Zig code
2 participants