code review 5569084: cmd/cgo: Emitting correct line number annotations by in...

cmd/cgo: Emitting correct line number annotations by introducing a new Writer
implementation (lineAdjuster) as a decorator sitting between go/printer.Fprint
and the .go file. Every line of generated code will be passed through an
instance of lineAdjuster and then output to the .cgo1.go file. lineAdjuster
will prpperly emit line number annotations in the middle of the .cgo1.go file.

Fixes issue 1047 and issue 2697

[Nan Deng, 2012-01-28 05:06:21]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
rsc@golang.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2012-01-31 16:05:02]

Thanks for working on this.  I very much like the
idea about the filter, but I am a little confused
about how this works.  I can see that it is resetting
the //line after the import "C" line, but I don't see it
resetting the //line after other omitted comments.
For example, if the input is

package p

/*
big comment
*/
import "C"

/*
another big comment
*/
func f() {}

then the current cgo1.go file does not include either
comment, and as far as I can tell this CL only
compensates for the first comment.

I think a complete fix might require some changes to the
printer itself.  Robert is working on some other things
in the printer, so now is probably not a good time to
make other changes.

Russ

[Nan Deng, 2012-02-02 04:36:01]

Hi Russ and other Go developers,

Thanks for your reply. I will explain a little below (Showing the 
correctness of this CL). It would be perfectly fine for my if you reject 
this patch. After all, for me, changing printer itself is a better 
solution to the problem itself. My solution is trying to not be too 
invasive and change less code.

Russ Cox wrote, On 01/31/2012 11:05 AM:
> Thanks for working on this.  I very much like the
> idea about the filter, but I am a little confused
> about how this works.  I can see that it is resetting
> the //line after the import "C" line, but I don't see it
> resetting the //line after other omitted comments.

Actually, it can deal with it. See the following code beloe:

package rand

/*
#include <stdlib.h>
#cgo LDFLAGS: -lm
*/
import "C"

/*
  * This
  * is
  * a comment
  * after import "C"
  */

func Random() int {
	return int(C.random())
}

The generated .cgo1.go is:

// Created by cgo - DO NOT EDIT

//line rand.go:1
package rand

//line rand.go:16
func Random() int {
	return int(_Cfunc_random())
}

You can see the second //line which annotated the correct line number.

I think what confused you is the member name I added to the File struct. 
The member FirstLineAfterImportC is actually, the first 
non-comment-non-blank line after import "C" (I explained it a little in 
the comment. But it seems did not work. I should put more information 
there). This value is set in ast.go:ReadGo() function.

Actually, I did make the mistake you mentioned when I first wrote the 
code. That's the reason why there is a member named 
FirstLineAfterImportC --- it was actually the first line after import 
"C" before. I just forget to change the name.

Thanks again for your reply.

Regards,
-Monnand

> For example, if the input is
>
> package p
>
> /*
> big comment
> */
> import "C"
>
> /*
> another big comment
> */
> func f() {}
>
> then the current cgo1.go file does not include either
> comment, and as far as I can tell this CL only
> compensates for the first comment.
>
> I think a complete fix might require some changes to the
> printer itself.  Robert is working on some other things
> in the printer, so now is probably not a good time to
> make other changes.
>
> Russ
>

[gri, 2012-02-02 18:01:03]

I can look at this later today or tomorrow. We have discussed a
special printer mode that inserts line information in the past; it
might be an easier fix.
- Robert

On Wed, Feb 1, 2012 at 8:35 PM, monnand@gmail.com <monnand@gmail.com> wrote:
> Hi Russ and other Go developers,
>
> Thanks for your reply. I will explain a little below (Showing the
> correctness of this CL). It would be perfectly fine for my if you reject
> this patch. After all, for me, changing printer itself is a better solution
> to the problem itself. My solution is trying to not be too invasive and
> change less code.
>
> Russ Cox wrote, On 01/31/2012 11:05 AM:
>
>> Thanks for working on this.  I very much like the
>> idea about the filter, but I am a little confused
>> about how this works.  I can see that it is resetting
>> the //line after the import "C" line, but I don't see it
>> resetting the //line after other omitted comments.
>
>
> Actually, it can deal with it. See the following code beloe:
>
> package rand
>
> /*
> #include <stdlib.h>
> #cgo LDFLAGS: -lm
> */
> import "C"
>
> /*
>  * This
>  * is
>  * a comment
>  * after import "C"
>  */
>
> func Random() int {
>        return int(C.random())
> }
>
> The generated .cgo1.go is:
>
>
> // Created by cgo - DO NOT EDIT
>
> //line rand.go:1
> package rand
>
> //line rand.go:16
> func Random() int {
>        return int(_Cfunc_random())
> }
>
> You can see the second //line which annotated the correct line number.
>
> I think what confused you is the member name I added to the File struct. The
> member FirstLineAfterImportC is actually, the first non-comment-non-blank
> line after import "C" (I explained it a little in the comment. But it seems
> did not work. I should put more information there). This value is set in
> ast.go:ReadGo() function.
>
> Actually, I did make the mistake you mentioned when I first wrote the code.
> That's the reason why there is a member named FirstLineAfterImportC --- it
> was actually the first line after import "C" before. I just forget to change
> the name.
>
> Thanks again for your reply.
>
> Regards,
> -Monnand
>
>
>> For example, if the input is
>>
>> package p
>>
>> /*
>> big comment
>> */
>> import "C"
>>
>> /*
>> another big comment
>> */
>> func f() {}
>>
>> then the current cgo1.go file does not include either
>> comment, and as far as I can tell this CL only
>> compensates for the first comment.
>>
>> I think a complete fix might require some changes to the
>> printer itself.  Robert is working on some other things
>> in the printer, so now is probably not a good time to
>> make other changes.
>>
>> Russ
>>
>

[rsc, 2012-02-02 19:03:10]

I was asking more about the general case of comments
being stripped, not just the one case of comments around
import "C".  If I pass the file below through cgo, the output
needs to have annotations so that the two reported compiler
errors (inside f and g) have correct line numbers.

Thanks.
Russ

package p

/*
big comment
*/
import "C"

/*
more comments
*/

func f() {
    /*
    another big comment
    */
    x := 1.0 + "hello"
}

func g() {
    /*
    another big comment
    */
    x := 1.0 + "hello"
}

[gri, 2012-02-08 01:34:03]

FYI: I haven't forgotten about this. Next up on the list.
- gri

On Thu, Feb 2, 2012 at 10:01 AM, Robert Griesemer <gri@golang.org> wrote:
> I can look at this later today or tomorrow. We have discussed a
> special printer mode that inserts line information in the past; it
> might be an easier fix.
> - Robert
>
> On Wed, Feb 1, 2012 at 8:35 PM, monnand@gmail.com <monnand@gmail.com> wrote:
>> Hi Russ and other Go developers,
>>
>> Thanks for your reply. I will explain a little below (Showing the
>> correctness of this CL). It would be perfectly fine for my if you reject
>> this patch. After all, for me, changing printer itself is a better solution
>> to the problem itself. My solution is trying to not be too invasive and
>> change less code.
>>
>> Russ Cox wrote, On 01/31/2012 11:05 AM:
>>
>>> Thanks for working on this.  I very much like the
>>> idea about the filter, but I am a little confused
>>> about how this works.  I can see that it is resetting
>>> the //line after the import "C" line, but I don't see it
>>> resetting the //line after other omitted comments.
>>
>>
>> Actually, it can deal with it. See the following code beloe:
>>
>> package rand
>>
>> /*
>> #include <stdlib.h>
>> #cgo LDFLAGS: -lm
>> */
>> import "C"
>>
>> /*
>>  * This
>>  * is
>>  * a comment
>>  * after import "C"
>>  */
>>
>> func Random() int {
>>        return int(C.random())
>> }
>>
>> The generated .cgo1.go is:
>>
>>
>> // Created by cgo - DO NOT EDIT
>>
>> //line rand.go:1
>> package rand
>>
>> //line rand.go:16
>> func Random() int {
>>        return int(_Cfunc_random())
>> }
>>
>> You can see the second //line which annotated the correct line number.
>>
>> I think what confused you is the member name I added to the File struct. The
>> member FirstLineAfterImportC is actually, the first non-comment-non-blank
>> line after import "C" (I explained it a little in the comment. But it seems
>> did not work. I should put more information there). This value is set in
>> ast.go:ReadGo() function.
>>
>> Actually, I did make the mistake you mentioned when I first wrote the code.
>> That's the reason why there is a member named FirstLineAfterImportC --- it
>> was actually the first line after import "C" before. I just forget to change
>> the name.
>>
>> Thanks again for your reply.
>>
>> Regards,
>> -Monnand
>>
>>
>>> For example, if the input is
>>>
>>> package p
>>>
>>> /*
>>> big comment
>>> */
>>> import "C"
>>>
>>> /*
>>> another big comment
>>> */
>>> func f() {}
>>>
>>> then the current cgo1.go file does not include either
>>> comment, and as far as I can tell this CL only
>>> compensates for the first comment.
>>>
>>> I think a complete fix might require some changes to the
>>> printer itself.  Robert is working on some other things
>>> in the printer, so now is probably not a good time to
>>> make other changes.
>>>
>>> Russ
>>>
>>

[rsc, 2012-02-08 03:37:38]

On Tue, Feb 7, 2012 at 20:34, Robert Griesemer <gri@golang.org> wrote:
> FYI: I haven't forgotten about this. Next up on the list.

Thanks!

[gri, 2012-02-08 23:53:55]

Hi there;

I think making this change in the go/printer will lead to a simpler,
faster, and more robust solution. I have a CL in the works that should
accomplish this. Essentially there's a new printer mode that - when
set - will cause the printer to emit a correcting //line comment
whenever the resulting output positions are different from the
original source.

Stay tuned.
- gri

On Wed, Feb 1, 2012 at 8:35 PM, monnand@gmail.com <monnand@gmail.com> wrote:
> Hi Russ and other Go developers,
>
> Thanks for your reply. I will explain a little below (Showing the
> correctness of this CL). It would be perfectly fine for my if you reject
> this patch. After all, for me, changing printer itself is a better solution
> to the problem itself. My solution is trying to not be too invasive and
> change less code.
>
> Russ Cox wrote, On 01/31/2012 11:05 AM:
>
>> Thanks for working on this.  I very much like the
>> idea about the filter, but I am a little confused
>> about how this works.  I can see that it is resetting
>> the //line after the import "C" line, but I don't see it
>> resetting the //line after other omitted comments.
>
>
> Actually, it can deal with it. See the following code beloe:
>
> package rand
>
> /*
> #include <stdlib.h>
> #cgo LDFLAGS: -lm
> */
> import "C"
>
> /*
>  * This
>  * is
>  * a comment
>  * after import "C"
>  */
>
> func Random() int {
>        return int(C.random())
> }
>
> The generated .cgo1.go is:
>
>
> // Created by cgo - DO NOT EDIT
>
> //line rand.go:1
> package rand
>
> //line rand.go:16
> func Random() int {
>        return int(_Cfunc_random())
> }
>
> You can see the second //line which annotated the correct line number.
>
> I think what confused you is the member name I added to the File struct. The
> member FirstLineAfterImportC is actually, the first non-comment-non-blank
> line after import "C" (I explained it a little in the comment. But it seems
> did not work. I should put more information there). This value is set in
> ast.go:ReadGo() function.
>
> Actually, I did make the mistake you mentioned when I first wrote the code.
> That's the reason why there is a member named FirstLineAfterImportC --- it
> was actually the first line after import "C" before. I just forget to change
> the name.
>
> Thanks again for your reply.
>
> Regards,
> -Monnand
>
>
>> For example, if the input is
>>
>> package p
>>
>> /*
>> big comment
>> */
>> import "C"
>>
>> /*
>> another big comment
>> */
>> func f() {}
>>
>> then the current cgo1.go file does not include either
>> comment, and as far as I can tell this CL only
>> compensates for the first comment.
>>
>> I think a complete fix might require some changes to the
>> printer itself.  Robert is working on some other things
>> in the printer, so now is probably not a good time to
>> make other changes.
>>
>> Russ
>>
>

[Nan Deng, 2012-02-09 01:18:32]

Robert Griesemer wrote, On 02/08/2012 06:53 PM:
> Hi there;
>
> I think making this change in the go/printer will lead to a simpler,
> faster, and more robust solution. I have a CL in the works that should
> accomplish this. Essentially there's a new printer mode that - when
> set - will cause the printer to emit a correcting //line comment
> whenever the resulting output positions are different from the
> original source.

This is definitely a better solution than mine. No need to say the bug 
in my code that Russ pointed.

Thanks,
-Nan
>
> Stay tuned.
> - gri
>
> On Wed, Feb 1, 2012 at 8:35 PM, monnand@gmail.com<monnand@gmail.com>  wrote:
>> Hi Russ and other Go developers,
>>
>> Thanks for your reply. I will explain a little below (Showing the
>> correctness of this CL). It would be perfectly fine for my if you reject
>> this patch. After all, for me, changing printer itself is a better solution
>> to the problem itself. My solution is trying to not be too invasive and
>> change less code.
>>
>> Russ Cox wrote, On 01/31/2012 11:05 AM:
>>
>>> Thanks for working on this.  I very much like the
>>> idea about the filter, but I am a little confused
>>> about how this works.  I can see that it is resetting
>>> the //line after the import "C" line, but I don't see it
>>> resetting the //line after other omitted comments.
>>
>>
>> Actually, it can deal with it. See the following code beloe:
>>
>> package rand
>>
>> /*
>> #include<stdlib.h>
>> #cgo LDFLAGS: -lm
>> */
>> import "C"
>>
>> /*
>>   * This
>>   * is
>>   * a comment
>>   * after import "C"
>>   */
>>
>> func Random() int {
>>         return int(C.random())
>> }
>>
>> The generated .cgo1.go is:
>>
>>
>> // Created by cgo - DO NOT EDIT
>>
>> //line rand.go:1
>> package rand
>>
>> //line rand.go:16
>> func Random() int {
>>         return int(_Cfunc_random())
>> }
>>
>> You can see the second //line which annotated the correct line number.
>>
>> I think what confused you is the member name I added to the File struct. The
>> member FirstLineAfterImportC is actually, the first non-comment-non-blank
>> line after import "C" (I explained it a little in the comment. But it seems
>> did not work. I should put more information there). This value is set in
>> ast.go:ReadGo() function.
>>
>> Actually, I did make the mistake you mentioned when I first wrote the code.
>> That's the reason why there is a member named FirstLineAfterImportC --- it
>> was actually the first line after import "C" before. I just forget to change
>> the name.
>>
>> Thanks again for your reply.
>>
>> Regards,
>> -Monnand
>>
>>
>>> For example, if the input is
>>>
>>> package p
>>>
>>> /*
>>> big comment
>>> */
>>> import "C"
>>>
>>> /*
>>> another big comment
>>> */
>>> func f() {}
>>>
>>> then the current cgo1.go file does not include either
>>> comment, and as far as I can tell this CL only
>>> compensates for the first comment.
>>>
>>> I think a complete fix might require some changes to the
>>> printer itself.  Robert is working on some other things
>>> in the printer, so now is probably not a good time to
>>> make other changes.
>>>
>>> Russ
>>>
>>
>

[gri, 2012-02-09 22:58:22]

Please see:

http://codereview.appspot.com/5643066

It would be great if you could verify that this works for cgo. I am
not 100% sure about the cgo specific changes. Thanks.
- gri

On Wed, Feb 8, 2012 at 5:18 PM, monnand@gmail.com <monnand@gmail.com> wrote:
> Robert Griesemer wrote, On 02/08/2012 06:53 PM:
>
>> Hi there;
>>
>> I think making this change in the go/printer will lead to a simpler,
>> faster, and more robust solution. I have a CL in the works that should
>> accomplish this. Essentially there's a new printer mode that - when
>> set - will cause the printer to emit a correcting //line comment
>> whenever the resulting output positions are different from the
>> original source.
>
>
> This is definitely a better solution than mine. No need to say the bug in my
> code that Russ pointed.
>
> Thanks,
> -Nan
>
>>
>> Stay tuned.
>> - gri
>>
>> On Wed, Feb 1, 2012 at 8:35 PM, monnand@gmail.com<monnand@gmail.com>
>>  wrote:
>>>
>>> Hi Russ and other Go developers,
>>>
>>> Thanks for your reply. I will explain a little below (Showing the
>>> correctness of this CL). It would be perfectly fine for my if you reject
>>> this patch. After all, for me, changing printer itself is a better
>>> solution
>>> to the problem itself. My solution is trying to not be too invasive and
>>> change less code.
>>>
>>> Russ Cox wrote, On 01/31/2012 11:05 AM:
>>>
>>>> Thanks for working on this.  I very much like the
>>>> idea about the filter, but I am a little confused
>>>> about how this works.  I can see that it is resetting
>>>> the //line after the import "C" line, but I don't see it
>>>> resetting the //line after other omitted comments.
>>>
>>>
>>>
>>> Actually, it can deal with it. See the following code beloe:
>>>
>>> package rand
>>>
>>> /*
>>> #include<stdlib.h>
>>> #cgo LDFLAGS: -lm
>>> */
>>> import "C"
>>>
>>> /*
>>>  * This
>>>  * is
>>>  * a comment
>>>  * after import "C"
>>>  */
>>>
>>> func Random() int {
>>>        return int(C.random())
>>> }
>>>
>>> The generated .cgo1.go is:
>>>
>>>
>>> // Created by cgo - DO NOT EDIT
>>>
>>> //line rand.go:1
>>> package rand
>>>
>>> //line rand.go:16
>>> func Random() int {
>>>        return int(_Cfunc_random())
>>> }
>>>
>>> You can see the second //line which annotated the correct line number.
>>>
>>> I think what confused you is the member name I added to the File struct.
>>> The
>>> member FirstLineAfterImportC is actually, the first non-comment-non-blank
>>> line after import "C" (I explained it a little in the comment. But it
>>> seems
>>> did not work. I should put more information there). This value is set in
>>> ast.go:ReadGo() function.
>>>
>>> Actually, I did make the mistake you mentioned when I first wrote the
>>> code.
>>> That's the reason why there is a member named FirstLineAfterImportC ---
>>> it
>>> was actually the first line after import "C" before. I just forget to
>>> change
>>> the name.
>>>
>>> Thanks again for your reply.
>>>
>>> Regards,
>>> -Monnand
>>>
>>>
>>>> For example, if the input is
>>>>
>>>> package p
>>>>
>>>> /*
>>>> big comment
>>>> */
>>>> import "C"
>>>>
>>>> /*
>>>> another big comment
>>>> */
>>>> func f() {}
>>>>
>>>> then the current cgo1.go file does not include either
>>>> comment, and as far as I can tell this CL only
>>>> compensates for the first comment.
>>>>
>>>> I think a complete fix might require some changes to the
>>>> printer itself.  Robert is working on some other things
>>>> in the printer, so now is probably not a good time to
>>>> make other changes.
>>>>
>>>> Russ
>>>>
>>>
>>
>

[Nan Deng, 2012-02-10 05:10:48]

Robert Griesemer wrote, On 02/09/2012 05:58 PM:
> Please see:
>
> http://codereview.appspot.com/5643066
>
> It would be great if you could verify that this works for cgo. I am
> not 100% sure about the cgo specific changes. Thanks.
> - gri

Sure. It's my honor. I will try some example code this week.

-Monnand

>
> On Wed, Feb 8, 2012 at 5:18 PM, monnand@gmail.com<monnand@gmail.com>  wrote:
>> Robert Griesemer wrote, On 02/08/2012 06:53 PM:
>>
>>> Hi there;
>>>
>>> I think making this change in the go/printer will lead to a simpler,
>>> faster, and more robust solution. I have a CL in the works that should
>>> accomplish this. Essentially there's a new printer mode that - when
>>> set - will cause the printer to emit a correcting //line comment
>>> whenever the resulting output positions are different from the
>>> original source.
>>
>>
>> This is definitely a better solution than mine. No need to say the bug in my
>> code that Russ pointed.
>>
>> Thanks,
>> -Nan
>>
>>>
>>> Stay tuned.
>>> - gri
>>>
>>> On Wed, Feb 1, 2012 at 8:35 PM, monnand@gmail.com<monnand@gmail.com>
>>>   wrote:
>>>>
>>>> Hi Russ and other Go developers,
>>>>
>>>> Thanks for your reply. I will explain a little below (Showing the
>>>> correctness of this CL). It would be perfectly fine for my if you reject
>>>> this patch. After all, for me, changing printer itself is a better
>>>> solution
>>>> to the problem itself. My solution is trying to not be too invasive and
>>>> change less code.
>>>>
>>>> Russ Cox wrote, On 01/31/2012 11:05 AM:
>>>>
>>>>> Thanks for working on this.  I very much like the
>>>>> idea about the filter, but I am a little confused
>>>>> about how this works.  I can see that it is resetting
>>>>> the //line after the import "C" line, but I don't see it
>>>>> resetting the //line after other omitted comments.
>>>>
>>>>
>>>>
>>>> Actually, it can deal with it. See the following code beloe:
>>>>
>>>> package rand
>>>>
>>>> /*
>>>> #include<stdlib.h>
>>>> #cgo LDFLAGS: -lm
>>>> */
>>>> import "C"
>>>>
>>>> /*
>>>>   * This
>>>>   * is
>>>>   * a comment
>>>>   * after import "C"
>>>>   */
>>>>
>>>> func Random() int {
>>>>         return int(C.random())
>>>> }
>>>>
>>>> The generated .cgo1.go is:
>>>>
>>>>
>>>> // Created by cgo - DO NOT EDIT
>>>>
>>>> //line rand.go:1
>>>> package rand
>>>>
>>>> //line rand.go:16
>>>> func Random() int {
>>>>         return int(_Cfunc_random())
>>>> }
>>>>
>>>> You can see the second //line which annotated the correct line number.
>>>>
>>>> I think what confused you is the member name I added to the File struct.
>>>> The
>>>> member FirstLineAfterImportC is actually, the first non-comment-non-blank
>>>> line after import "C" (I explained it a little in the comment. But it
>>>> seems
>>>> did not work. I should put more information there). This value is set in
>>>> ast.go:ReadGo() function.
>>>>
>>>> Actually, I did make the mistake you mentioned when I first wrote the
>>>> code.
>>>> That's the reason why there is a member named FirstLineAfterImportC ---
>>>> it
>>>> was actually the first line after import "C" before. I just forget to
>>>> change
>>>> the name.
>>>>
>>>> Thanks again for your reply.
>>>>
>>>> Regards,
>>>> -Monnand
>>>>
>>>>
>>>>> For example, if the input is
>>>>>
>>>>> package p
>>>>>
>>>>> /*
>>>>> big comment
>>>>> */
>>>>> import "C"
>>>>>
>>>>> /*
>>>>> another big comment
>>>>> */
>>>>> func f() {}
>>>>>
>>>>> then the current cgo1.go file does not include either
>>>>> comment, and as far as I can tell this CL only
>>>>> compensates for the first comment.
>>>>>
>>>>> I think a complete fix might require some changes to the
>>>>> printer itself.  Robert is working on some other things
>>>>> in the printer, so now is probably not a good time to
>>>>> make other changes.
>>>>>
>>>>> Russ
>>>>>
>>>>
>>>
>>
>