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

Remove defer Statement #10

Merged
merged 1 commit into from Oct 21, 2020
Merged

Remove defer Statement #10

merged 1 commit into from Oct 21, 2020

Conversation

gsquire
Copy link
Contributor

@gsquire gsquire commented Oct 21, 2020

This small patch removes a defer statement inside of a for loop and calls Close after the scanner is done reading the file.

This small patch removes a defer statement inside of a for loop and
calls Close after the scanner is done reading the file.
@muesli
Copy link
Owner

muesli commented Oct 21, 2020

Thanks for PR, @gsquire! I'm not sure however if this is fixing or improving anything. Can you elaborate?

Edit: I guess it would make a difference if you got a larger amount of dictionaries and want to prevent keeping the file handles open. Makes sense.

@muesli muesli merged commit bbdf2d0 into muesli:master Oct 21, 2020
@gsquire gsquire deleted the defer-in-loop branch October 21, 2020 04:42
@muesli
Copy link
Owner

muesli commented Oct 21, 2020

Thank you!

@gsquire
Copy link
Contributor Author

gsquire commented Oct 21, 2020

You're welcome! My reasoning was that the defer calls will stack up and not reference the correct open file when they are finally invoked. Thus you would leak open files.

@muesli muesli added the bug label Oct 21, 2020
@muesli
Copy link
Owner

muesli commented Oct 21, 2020

Hah, I see you've even written your own little detector: https://github.com/gsquire/dll

Kudos, nice work!

@gsquire
Copy link
Contributor Author

gsquire commented Oct 21, 2020

Thanks, hope you can find it useful :)

@muesli
Copy link
Owner

muesli commented Oct 21, 2020

Note: I don't think that would have been an issue here, as file is declared in the scope of the for loop.

Also, unless I'm entirely mistaken, this should also only be a problem if the actual loop-value were to be evaluated in the deferred call.

For example, this isn't a problem:

package main

import "fmt"

type foo struct {
	name string
}

func (f foo) print() {
	fmt.Println(f.name)
}

func main() {
	var f []foo

	f = append(f, foo{name: "a"})
	f = append(f, foo{name: "b"})
	f = append(f, foo{name: "c"})

	for _, v := range f {
		defer v.print()
	}
}

Output (as expected):

c
b
a

This however would lead to problems:

package main

import "fmt"

type foo struct {
	name string
}

func (f foo) print() {
	fmt.Println(f.name)
}

func main() {
	var f []foo

	f = append(f, foo{name: "a"})
	f = append(f, foo{name: "b"})
	f = append(f, foo{name: "c"})

	for _, v := range f {
		defer func() {
			v.print()
		}()
	}
}

Output:

c
c
c

Maybe that's something to take into account for dll.

@gsquire
Copy link
Contributor Author

gsquire commented Oct 21, 2020

Yeah, I think the idea is that they could pile up in something like a server with an infinite loop for example. It'd be hard to check that semantically within the tool but it's something cool to explore. There is a way around it as well.

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

Successfully merging this pull request may close these issues.

None yet

2 participants