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

WIP: New inspection: Obtaining address of a for loop variable #2422

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

deadok22
Copy link
Contributor

Please, don't merge it now - it's a work in progress.

Today I've spent about an hour looking for a bug introduced by obtaining address of a for loop variable. This inspection could have helped to avoid that bug, or at the very least, it'd have helped me find it =)

Please, let me know if my vision of the problem is incomplete or if this inspection is too loud.

@deadok22 deadok22 force-pushed the address-of-for-loop-variable branch from 58d00af to 7845df6 Compare March 30, 2016 05:41
@zolotov zolotov self-assigned this Mar 30, 2016
@zolotov
Copy link
Contributor

zolotov commented Mar 30, 2016

Not sure that it's necessary inspection. As far as I understand it will compain about following code:

package main

import "fmt"

func doSomethingWithPointer(pointer *int) {
    fmt.Println(*pointer)
}

func main() {
    for _, i := range []int{1, 2, 3} {
        doSomethingWithPointer(&i)
    }
}

but in my opinion it's more or less valid case and its output looks logical to me:

1
2
3

@deadok22
Copy link
Contributor Author

Your example is legit.

But it can be a problem if a pointer is stored somewhere. The pointer is the same on each iteration of the loop and it can be a source of hard-to-find bugs.

@zolotov
Copy link
Contributor

zolotov commented Mar 30, 2016

My point is that other functions might require pointer-parameters and you have to obtain address in these cases, and I think that IDEA shouldn't complain about it. gometalinter doesn't complain about it neither.

@ignatov
Copy link
Contributor

ignatov commented Mar 30, 2016

BTW, you can add it and disable by default.

@deadok22
Copy link
Contributor Author

@zolotov that's true, that's why I asked for opinions on whether this inspection is too loud.

So, yes, I can either make it disabled by default, or I can use scripting extensions to have it for myself only.

@zolotov
Copy link
Contributor

zolotov commented Mar 30, 2016

Disabled by default inspection looks good to me

@zolotov
Copy link
Contributor

zolotov commented Apr 13, 2016

@deadok22 can it be merge now? with disabling by default. If so, please rebase and disable the inspection by default.

@deadok22
Copy link
Contributor Author

No, I still have a TODO there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants