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

lang: ast: Remove redundant nil check #731

Merged
merged 1 commit into from
Feb 8, 2024
Merged

lang: ast: Remove redundant nil check #731

merged 1 commit into from
Feb 8, 2024

Conversation

Juneezee
Copy link
Contributor

From the Go specification 1:

"1. ... For a nil slice, the number of iterations is 0."

Therefore, an additional nil check for around the loop is unnecessary.

/cc @purpleidea for review

Footnotes

  1. https://go.dev/ref/spec#For_range

Copy link
Contributor Author

@Juneezee Juneezee left a comment

Choose a reason for hiding this comment

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

Small tip: Hide whitespace changes to make the diff easier to read

image


image

@Juneezee
Copy link
Contributor Author

@purpleidea I apologize for the nil pointer panic introduced in the first commit. I have pushed a fix for the panic. Please approve the CI runs again 🙇‍♂️ .

CI runs in my fork are now green https://github.com/Juneezee/mgmt/actions/runs/7677576810

@purpleidea
Copy link
Owner

@Juneezee Thanks for the patch-- just a quick question or two if you don't mind:

  1. What tool did you use to find these changes?
  2. Are you interested in making some bigger mgmt patches?
  3. Thanks!
    image

@Juneezee
Copy link
Contributor Author

Juneezee commented Jan 28, 2024

just a quick question or two if you don't mind:

@purpleidea Yup, I don't mind 😄

  1. What tool did you use to find these changes?

GitHub search. I was searching for "abstract syntax tree" to find some code for learning purposes. This repository is one of the top results on the first page.

This was the search query that I used: "abstract syntax tree" lang:Go -repo:golang/go

image

  1. Are you interested in making some bigger mgmt patches?

Yes! Absolutely!

  1. Thanks!
    image

1997, my birth year Oops, largest even prime number 😄, 2

@purpleidea
Copy link
Owner

What tool did you use to find these changes?

Thanks for the info-- sorry what I mean was, what linting tool did you use to find the specific code changes to make, or did you read through the whole file and decide on them manually?

Are you interested in making some bigger mgmt patches?
Yes! Absolutely!

Okay great! Want to give me a quick summary of your strengths and weaknesses in golang, if you're comfortable with golang concurrency, and what you'd like to work on. Eg: parser stuff, a resource, a function, concurrency bugs etc...

I can help you choose and mentor you on your patches. Cheers!

@Juneezee
Copy link
Contributor Author

Juneezee commented Jan 28, 2024

Thanks for the info-- sorry what I mean was, what linting tool did you use to find the specific code changes to make, or did you read through the whole file and decide on them manually?

I did skim through the whole file. I use GoLand and I noticed the slice declaration warning, which is just 1 line above my change.

image

Okay great! Want to give me a quick summary of your strengths and weaknesses in golang, if you're comfortable with golang concurrency, and what you'd like to work on. Eg: parser stuff, a resource, a function, concurrency bugs etc...

I can help you choose and mentor you on your patches. Cheers!

I would say I'm pretty confident in normal day-to-day Go programming, and I'm comfortable with Go concurrency too.

However, I lack experience with project architecture and engineering practices (stuff like that). If there are issues or feature requests that you think are beginner-friendly, feel free to assign them to me and I am happy to work on it 😄

Thank you very much!

@purpleidea
Copy link
Owner

slice declaration warning

Interesting, I didn't consider this a bad practice. I'll look into it more.

feel free to assign them to me and I am happy to work on it 😄

Okay, since you mentioned AST's and compilers, I wrote up an issue over here... #732 If you're interested in that one, LMK and comment over there that you're working on it. If you want something different, please LMK.

Copy link
Owner

@purpleidea purpleidea left a comment

Choose a reason for hiding this comment

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

Looks good, just the one change throughout as mentioned and please rebase into a single commit against master. Thank you!

lang/ast/structs.go Outdated Show resolved Hide resolved
From the Go specification [1]:

  "1. ... For a nil slice, the number of iterations is 0."

Therefore, an additional nil check for around the loop is unnecessary.

[1]: https://go.dev/ref/spec#For_range

Signed-off-by: Eng Zer Jun <[email protected]>
@purpleidea
Copy link
Owner

LGTM, let's see what CI says... Thanks!

@purpleidea purpleidea merged commit b09b21e into purpleidea:master Feb 8, 2024
3 checks passed
@purpleidea
Copy link
Owner

Merged, thank you!

What's next?

@Juneezee
Copy link
Contributor Author

Juneezee commented Feb 8, 2024

Merged, thank you!

What's next?

I'll be looking into #732, and I hope I can solve it

@Juneezee Juneezee deleted the redundant-nil-check branch February 8, 2024 21:18
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.

3 participants