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

go/internal/gccgoimporter: misleading use of pkg parameter #19407

Closed
mvdan opened this issue Mar 5, 2017 · 4 comments
Closed

go/internal/gccgoimporter: misleading use of pkg parameter #19407

mvdan opened this issue Mar 5, 2017 · 4 comments

Comments

@mvdan
Copy link
Member

@mvdan mvdan commented Mar 5, 2017

In

func (p *parser) discardDirectiveWhileParsingTypes(pkg *types.Package) {
, we have:

func (p *parser) discardDirectiveWhileParsingTypes(pkg *types.Package) {
        for {
                switch p.tok {
                case ';':
                        return
                case '<':
                        p.parseType(p.pkg)
                case scanner.EOF:
                        p.error("unexpected EOF")
                default:
                        p.next()
                }
        }
}

This seems off. Either we want to use pkg instead of p.pkg, or the parameter is unnecessary. I would assume the former is the correct solution, but opening this issue first to confirm that.

Found via https://github.com/mvdan/unparam.

@mvdan
Copy link
Member Author

@mvdan mvdan commented Mar 5, 2017

I've filed https://go-review.googlesource.com/c/37839/ to make it use pkg instead of p.pkg.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 5, 2017

CL https://golang.org/cl/37839 mentions this issue.

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Mar 5, 2017

It seems most of parser methods take pkg *types.Package as first parameter. I've looked at the call graph, and if you trace it back to where it's being provided, it all comes from p.pkg. So in a way, all those pkg parameters are largely redundant.

Doing anything about that is probably out of scope for that CL, because you'd need to know what the owner of the code is planning to do next, but I just wanted to point this out.

@mvdan
Copy link
Member Author

@mvdan mvdan commented Mar 6, 2017

@shurcooL I assumed that to be the case, otherwise this pkg and p.pkg mixup should be breaking things. I opted for the smallest CL possible for now.

@golang golang locked and limited conversation to collaborators Mar 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants