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

cmd/compile: fix line number during yyerror in pragcgo #18882

Closed
rsc opened this issue Feb 1, 2017 · 5 comments
Closed

cmd/compile: fix line number during yyerror in pragcgo #18882

rsc opened this issue Feb 1, 2017 · 5 comments
Labels
FrozenDueToAge
Milestone
Go1.9Early

Comments

@rsc
Copy link
Contributor

rsc commented Feb 1, 2017

Merged position changes from dev.inline into master.
The only part I wasn't sure about was how to translate this one line added to master since the dev.inline branched forked off. I commented it with TODO(gri) instead:

case strings.HasPrefix(text, "go:cgo_"):
	// TODO(gri): lineno = p.baseline + int32(line) - 1 // pragcgo may call yyerror
	p.pragcgobuf += pragcgo(text)
	fallthrough // because of //go:cgo_unsafe_args
@rsc rsc added this to the Go1.9Early milestone Feb 1, 2017
@odeke-em
Copy link
Member

odeke-em commented Feb 1, 2017

@rsc I had mentioned in #18459 (comment), that I had encountered this and perhaps we can fix it like I did here https://go-review.googlesource.com/c/34562

p.error(syntax.Error{Pos: pos, Line: line, Msg: err.Error()})

We didn't include the suggested change because it was very late in the Go1.8 cycle and my
suggestion was a lot larger than the two liner change @griesemer made in
https://go-review.googlesource.com/c/34721/

@griesemer
Copy link
Contributor

@odeke-em While your change is maybe the right approach long-term, it's too large for the (small) issue, and doesn't fit the current error handling model (which is going to change anyway, down the road).

It appears that the test I added with https://go-review.googlesource.com/c/34721/ is passing, so we may not have an issue here, but I will investigate.

@gopherbot
Copy link

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

@griesemer
Copy link
Contributor

@odeke-em It turns out your fix is actually the correct one - my initial approach won't work anymore due to the concurrency we have now (pragcgo is called concurrently since files are parsed concurrently). Working on a correct solution...

@odeke-em
Copy link
Member

odeke-em commented Feb 2, 2017

Cool, glad to know it worked 🍻

@golang golang locked and limited conversation to collaborators Feb 2, 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
Go1.9Early
Development

No branches or pull requests
4 participants
@rsc @odeke-em @griesemer @gopherbot