code review 979046: Don't generate import statements for unused imports.

Don't generate import statements for unused imports.

[dsymonds, 2010-04-30 05:08:33]

Hello r,

I'd like you to review this change.

[r, 2010-04-30 05:56:31]

btw: i have a huge edit coming that might affect how you'd approach this

http://codereview.appspot.com/979046/diff/2001/6
File compiler/main.go (right):

http://codereview.appspot.com/979046/diff/2001/6#newcode265
compiler/main.go:265: buf *bytes.Buffer
why??

http://codereview.appspot.com/979046/diff/2001/6#newcode587
compiler/main.go:587: // Generate header and after finding out exactly which
packages are needed.
s/exactly //

http://codereview.appspot.com/979046/diff/2001/6#newcode592
compiler/main.go:592: g.buf.Write(body.Bytes())
that's really ugly.

http://codereview.appspot.com/979046/diff/2001/7
File compiler/testdata/test.proto (right):

http://codereview.appspot.com/979046/diff/2001/7#newcode35
compiler/testdata/test.proto:35: import "multi1.proto";  // unused import
do we really need to deal with this?

[dsymonds, 2010-04-30 06:02:35]

On Fri, Apr 30, 2010 at 3:56 PM,  <r@golang.org> wrote:

> btw: i have a huge edit coming that might affect how you'd approach this

Okay, I can hold onto this CL until yours lands. It's easy enough to
reimplement.


> http://codereview.appspot.com/979046/diff/2001/6#newcode265
> compiler/main.go:265: buf *bytes.Buffer
> why??

I couldn't figure out how to swap out the bytes.Buffer as an anonymous field.

> http://codereview.appspot.com/979046/diff/2001/6#newcode587
> compiler/main.go:587: // Generate header and after finding out exactly
> which packages are needed.
> s/exactly //

Done.

> http://codereview.appspot.com/979046/diff/2001/6#newcode592
> compiler/main.go:592: g.buf.Write(body.Bytes())
> that's really ugly.

I don't think it's so bad. Is there a better approach I'm overlooking?

> http://codereview.appspot.com/979046/diff/2001/7#newcode35
> compiler/testdata/test.proto:35: import "multi1.proto";  // unused
> import
> do we really need to deal with this?

We will, internally. We discussed this approach already, didn't we?


Dave.

[r2, 2010-04-30 06:09:17]

On Apr 29, 2010, at 11:02 PM, David Symonds wrote:

> On Fri, Apr 30, 2010 at 3:56 PM,  <r@golang.org> wrote:
> 
>> btw: i have a huge edit coming that might affect how you'd approach this
> 
> Okay, I can hold onto this CL until yours lands. It's easy enough to
> reimplement.
> 
> 
>> http://codereview.appspot.com/979046/diff/2001/6#newcode265
>> compiler/main.go:265: buf *bytes.Buffer
>> why??
> 
> I couldn't figure out how to swap out the bytes.Buffer as an anonymous field.

it's called g.Buffer.

>> http://codereview.appspot.com/979046/diff/2001/6#newcode587
>> compiler/main.go:587: // Generate header and after finding out exactly
>> which packages are needed.
>> s/exactly //
> 
> Done.
> 
>> http://codereview.appspot.com/979046/diff/2001/6#newcode592
>> compiler/main.go:592: g.buf.Write(body.Bytes())
>> that's really ugly.
> 
> I don't think it's so bad. Is there a better approach I'm overlooking?

it feel very wrong to start generating text	 knowing you're going to squirt
something out at the beginning once you're done.  why not arrange for the
inevitable?

one approach is to walk the tree first and mark what you need, but that is
fragile with respect to what's actually generated, so i think this
mark-as-you-go approach is probably right.  but i would like a cleaner way to
handle the i/o.   you could have a sort of 'save this' method that stores the
elements in a growing slice of slice of bytes. then the return is just a
bytes.Add in the right sequence.  that approach allows for flexibility if needs
change.

>> http://codereview.appspot.com/979046/diff/2001/7#newcode35
>> compiler/testdata/test.proto:35: import "multi1.proto";  // unused
>> import
>> do we really need to deal with this?
> 
> We will, internally. We discussed this approach already, didn't we?

i know, i'm just complaining. i hate having to make my work messier because of
others' lousy designs.
> 
> 
> Dave.

[dsymonds, 2010-04-30 06:17:30]

On Fri, Apr 30, 2010 at 4:09 PM, Rob 'Commander' Pike <r@google.com> wrote:

> i know, i'm just complaining. i hate having to make my work messier because of
others' lousy designs.

Another approach that I initially tried was to just output the imports
at the end of the file. The language spec (and compiler) doesn't allow
that, though it would have made this even simpler.

I'll do that slice of slice idea once your CL is done and in.

Actually, now that I think about it, that's pretty much what a Cord
is, and it would probably be generically useful. I could see a new
Cord type as part of the bytes package (and perhaps a corresponding
one in strings) that wraps a list. Would you be amenable to such an
addition?


Dave.

[dsymonds, 2010-05-03 00:02:21]

It's back to using bytes.Buffer. PTAL.

[r, 2010-05-05 00:50:27]

LGTM

http://codereview.appspot.com/979046/diff/25001/26002
File compiler/generator/generator.go (right):

http://codereview.appspot.com/979046/diff/25001/26002#newcode624
compiler/generator/generator.go:624: log.Stderr("discarding unused import: ",
filename)
log messages should say which program is generating them.
protoc-gen-go: discarding...

[dsymonds, 2010-05-05 00:53:23]

http://codereview.appspot.com/979046/diff/25001/26002
File compiler/generator/generator.go (right):

http://codereview.appspot.com/979046/diff/25001/26002#newcode624
compiler/generator/generator.go:624: log.Stderr("discarding unused import: ",
filename)
On 2010/05/05 00:50:27, r wrote:
> log messages should say which program is generating them.
> protoc-gen-go: discarding...

Done.

[dsymonds, 2010-05-05 00:54:53]

*** Submitted as 152c47d424f9 ***

Don't generate import statements for unused imports.

R=r
http://codereview.appspot.com/979046