goccmack / gocc

Parser / Scanner Generator
Other
622 stars 48 forks source link

all: update for Go1.12 and drop <Go1.10 support #83

Closed kortschak closed 5 years ago

kortschak commented 5 years ago

Please take a look.

Closes #81.

awalterschulze commented 5 years ago

Thank you for this, this looks great :)

Is it possible to see some different code generation times, between the two versions, using strings.Builder and bytes.Buffer?

kortschak commented 5 years ago

It's difficult to benchmark this. I tried using /usr/bin/time for code gen of the gonum dot format parser and the differences were entirely in the noise (both around ~0.28s elapsed for both on my machine), but it's a fairly small grammar. If you can point me to a more complex grammar I'll have another go.

mewmew commented 5 years ago

@kortschak, you can try using this grammar for benchmarking. Without newlines and comments, it is about 2100 lines of Gocc grammar.

https://gist.github.com/mewmew/84d2f945acbb879ff78e6e5ba3c6248b#file-ll-bnf

On my laptop, it takes ~3.5 minutes to generate lexers and parsers for the grammar using Gocc. This is on rev 8d8e0d2b823b46a90bf00233a854b95a4665ea4d.

u@x1 ~/D/a2> time gocc ll.bnf 
real 212.45
user 240.38
sys 1.89
mewmew commented 5 years ago

Probably within noise limits, but this is the run time I got on your strings.Builder branch.

u@x1 ~/D/a2> time gocc ll.bnf
real 219.35
user 240.10
sys 2.08
kortschak commented 5 years ago

I see a consistent couple of seconds difference over 3min40sec. In favor of the old code. I can drop that change out and just do the version updates and go.mod addition if you would like.

mewmew commented 5 years ago

I can drop that change out and just do the version updates and go.mod addition if you would like.

Personally, I'm fine with replacing bytes.Buffer with strings.Builder. The purpose of strings.Builder if I understood it correctly is to reduce memory allocations. It may be that bytes.Buffer has been part of Go for longer and thus been optimized (somehow..). However, it should be reasonable to believe that strings.Builder will catch up in the future? At least based on those thoughts, I'd say, let the strings.Builder stay.

mewmew commented 5 years ago

@awalterschulze, what are your thoughts? I'm fine with merging this PR as is.

awalterschulze commented 5 years ago

I am also fine with merging strings.Builder and the updated go version.

I don't think we need go.mod or go.sum though. As far as I know the debate over a dependency manager is still something that go is working through. And these dependencies are only used for checking our build and not a real dependency. And also gocc is not a library, it is a code generator and the generated code does not depend on gocc, so I don't see a need for this to be a module, but maybe I am wrong?

Maybe my suspicions here are misplaced, I am just very uncomfortable with how dependencies and modules are handled by the go team and I want to wait to see how it plays out.

kortschak commented 5 years ago

The presence of go.mod and go.sum is to address issues like this. Failure here - this fails for go1.12.

I am also uncomfortable with modules (though getting less so), but they are part of reality now.

kortschak commented 5 years ago

The role of modules in tools like gocc is that with modules it can be got at a particular version to ensure reproducible builds (I'm actually looking forward to this since at the moment we have a script to do this).

awalterschulze commented 5 years ago

Are you sure we can't just release a semantic version of gocc to github and then you can require ( github.com/goccmack/gocc v2.4.0 ) Otherwise maybe we can discuss go.sum and go.mod in a separate commit?

kortschak commented 5 years ago

No, that's not the problem. See the failure error:

go: cannot find main module, but found .git/config in /home/travis/gopath/src/github.com/goccmack/gocc
    to create a module there, run:
    go mod init
The command "./.travis/install-gocc.sh 0e2cfc030005b281b2e5a2de04fa7fe1d5063722" failed and exited with 1 during .
awalterschulze commented 5 years ago

Okay maybe I am just out of the loop with Go's new module system. If others that are more up to data with Go, think go.mod and go.sum are good, then I approve the rest of the commit.

But out of interest, what is go mod's plan with repositories that don't have a go.mod file?

kortschak commented 5 years ago

But out of interest, what is go mod's plan with repositories that don't have a go.mod file?

TBH I'm still not wildly happy with the level of documentation/education wrt module systems' adoption and this is something that I have not seen discussed anywhere. There is discussion of how no-go.mod repos work when they are dependencies of other packages, but nothing that I have seen about how it works in this particular case where it's a stand alone system. I'll ask on go-nuts later today and report back.

For interest, the no-go.mod repo case for v0/v1 just works. For v2+ no-go.mod repos see the +incompatible part of the Semantic Import Versioning section of the modules wiki.

awalterschulze commented 5 years ago

@goccmack @shivansh ^^

kortschak commented 5 years ago

For information. I have found the commit that causes the requirement. It is golang/go@65c2069a9f30cb6fa2c512d17dc0ad654d621da9. This is after go1.12.

goccmack commented 5 years ago

I have been using go modules since last year with good results. I expect the documentation to improve this year. So I am happy with the change.

awalterschulze commented 5 years ago

Ok then it is just me, who hasn't used go in a few months. Sorry for causing the delay and thank you @kortschak for bringing this project up to date.

kortschak commented 5 years ago

Thanks

kortschak commented 5 years ago

The bytes=>strings change broke things. I'll send a PR in a few minutes. I'm surprised this wasn't caught by tests.