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

Use IsZero and "zero" instead of Nil and "nil" for pointer-like types (except arena.Pointer) #402

Merged
merged 6 commits into from
Dec 19, 2024

Conversation

mcy
Copy link
Member

@mcy mcy commented Dec 19, 2024

This PR is mostly a cosmetic change.

However, this was also an opportunity to ensure that every accessor-like method on AST types returns zero, instead of randomly panicking as is the case now. This is enforced by a test.

@mcy mcy requested a review from jhump December 19, 2024 19:23
Copy link
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

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

Very thorough! I like it.

@@ -23,7 +23,7 @@ import (
)

const (
DeclKindNil DeclKind = iota
DeclKindZero DeclKind = iota
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could call the zero kinds "Invalid" or "Undefined"? So a zero value has an invalid/undefined kind. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, invalid is fine.

Copy link
Member

Choose a reason for hiding this comment

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

This comment was really directed at all kinds, not just DeclKind. Sorry if that wasn't clear. See #405.

@mcy mcy enabled auto-merge (squash) December 19, 2024 23:25
@mcy mcy merged commit c3e07ad into main Dec 19, 2024
8 checks passed
@mcy mcy deleted the mcy/iszero branch December 19, 2024 23:28
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