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

Jump to bottom

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
Labels
FrozenDueToAge

Comments

@mvdan
Copy link
Member

mvdan commented Mar 5, 2017 •
edited

In

go/src/go/internal/gccgoimporter/parser.go

Line 736 in d445136

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 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

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

@dmitshur
Copy link
Contributor

dmitshur commented Mar 5, 2017 •
edited

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 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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dmitshur @mvdan @gopherbot