pomack / thrift4go

Apache Thrift for the Go Language
129 stars 23 forks source link

Go1 error "err shadowed during return" #11

Open petar opened 12 years ago

petar commented 12 years ago

I installed the Thrift 0.8.0 package from "brew".

The Go code generated from this package does not compile due to numerous identical compiler errors:

"err is shadowed during return"

Does this repository here have this problem fixed? (I don't really know which github version of thrift4go is actually in Thrift 0.8.0)

Thanks Petar

pomack commented 12 years ago

1) The Go code and Go generator in the official Thrift package came from this repository, but hasn't been updated in the official Thrift package since its original release. Apache has said they prefer release builds and since Go1 is the first real "release" build of Go, I haven't submitted the changes yet. I'll get around to it if enough people push me to ;) 2) The library code in the github repository is tested to work with Go1. 3) The code generator in the github repository has been tested to work with Go1 4) I didn't realize Thrift 0.8.0 was out. 5) I've been testing with Thrift 0.7.something, although others may have been testing with the latest version. So I'm not sure if the generator code will necessarily work with Thrift 0.8.0. You're more than welcome to try. Let me know the results if you do.

petar commented 12 years ago

I substituted your tgo....cc file into the newest Thrift sources and made thrift.

Then I used that thrift to generate some code. It basically works. You've fixed the bug I had a problem with. I needed to make some minor changes in the "import" clause of the generated code.

pomack commented 12 years ago

What changes did you have to make to the import clause? Were there extra imports or was the line itself incorrect?

petar commented 12 years ago

A few small things:

(a) I had one thrift file, say a.thrift, importing another, say b.thrift.

In the generated code for a, there was an import statement for the generated code of b, but then it was being used without the package name in front. So I needed to change

import "b"

to

import . "b"

and it worked.

(b) Another small issue was that my thrift file had some field named "error" which got used as field name in Go, and since it is a reserved word, it created all sorts of compiler errors. I just renamed the field.

(c) The generated code seems to put "thriftlib" in front of the names of any imported other auto-generated packages.

Since, in my case, these packages go somewhere inside big source tree of packages, I had to manually replace "thriftlib" with the place I put the auto-generated files, something like "project_name/sub_project_name/thrift_autogen/".

Thanks Petar

On 12 April 2012 10:09, Aalok Shah reply@reply.github.com wrote:

What changes did you have to make to the import clause?  Were there extra imports or was the line itself incorrect?


Reply to this email directly or view it on GitHub: https://github.com/pomack/thrift4go/issues/11#issuecomment-5091741

pomack commented 12 years ago

Issue b should now be resolved. While the spec at http://golang.org/ref/spec#Keywords doesn't mention error as a reserved word, I assume it now is and this just wasn't updated. I've updated the necessary function with a one-liner and it should work now.

pomack commented 12 years ago

Issue a) I don't like the workaround as there could be object names that are hidden due to name conflicts. This type of stuff should also work out of the box. I know where the generated code needs to change, but will need to test this out to ensure I don't break anything.

Can you send me a couple sample files so I can confirm what changes I will make work properly?

pomack commented 12 years ago

Issue c) I wanted to make sure I didn't overwrite some important packages (like os, thrift, etc.) when I installed the library files, so I made all libraries install under the GOROOT/pkg/arch/thriftlib/ directory (back from the r57 days). Starting with Go1, build/install uses the new "go" command that does not require Makefiles but a GOPATH. Non-core library installs (like thrift) should be installed into a local directory along the GOPATH not into the GOROOT/pkg/arch/ directories like they were before. I need to change the directory structure of the generated code, and the Makefiles (since I still like Makefiles, especially if you need to compile from a bunch of directories).

This will take longer to confirm everything is working properly. I also won't push out a build to Apache until issues a) and c) are resolved appropriately.

petar commented 12 years ago

Sounds good.

--Petar

On 13 April 2012 02:25, Aalok Shah reply@reply.github.com wrote:

Issue c) I wanted to make sure I didn't overwrite some important packages (like os, thrift, etc.) when I installed the library files, so I made all libraries install under the GOROOT/pkg//thriftlib/ directory (back from the r57 days).  Starting with Go1, build/install uses the new "go" command that does not require Makefiles but a GOPATH.  Non-core library installs (like thrift) should be installed into a local directory along the GOPATH not into the GOROOT/pkg// directories like they were before.  I need to change the directory structure of the generated code, and the Makefiles (since I still like Makefiles, especially if you need to compile from a bunch of directories).

This will take longer to confirm everything is working properly.  I also won't push out a build to Apache until all issues a) and c) are resolved appropriately.


Reply to this email directly or view it on GitHub: https://github.com/pomack/thrift4go/issues/11#issuecomment-5109083

pomack commented 12 years ago

Hey Petar,

Matt Proud has been making a lot of changes to thrift4go including a script to merge the source trees. Have you had a chance to try it out?

pomack commented 11 years ago

Hey Petar,

Is this still an issue?