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

Support switch label clauses. Fixes #693 #694

Merged

Conversation

kamphaus
Copy link
Contributor

@kamphaus kamphaus commented Apr 28, 2018

Fixes #693


This change is Reviewable

@codecov
Copy link

codecov bot commented Apr 28, 2018

Codecov Report

Merging #694 into master will decrease coverage by 0.08%.
The diff coverage is 70.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #694      +/-   ##
==========================================
- Coverage   80.87%   80.79%   -0.09%     
==========================================
  Files         166      166              
  Lines        9351     9478     +127     
==========================================
+ Hits         7563     7658      +95     
- Misses       1369     1388      +19     
- Partials      419      432      +13
Impacted Files Coverage Δ
transpiler/switch.go 75.75% <70.34%> (-3.07%) ⬇️
program/program.go 89.4% <0%> (+2.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95503a2...0f6a60b. Read the comment docs.

@kamphaus kamphaus force-pushed the fix-693-switch-label-clause branch 2 times, most recently from 02db90c to 3e1c71c Compare May 6, 2018 08:55
@kamphaus kamphaus force-pushed the fix-693-switch-label-clause branch 2 times, most recently from 4031cc1 to 5b59f0a Compare May 14, 2018 06:42
@kamphaus kamphaus force-pushed the fix-693-switch-label-clause branch from 5b59f0a to 2cff2ba Compare May 24, 2018 06:56
@elliotchance
Copy link
Owner

You have accidentally committed the vendor folder.

@kamphaus kamphaus force-pushed the fix-693-switch-label-clause branch from ed0f766 to c43a241 Compare May 29, 2018 06:34
@kamphaus
Copy link
Contributor Author

The vendor folder was meant to be committed.
This could be part of a greater discussion how to do dependency management in Go.
For the moment c2go has only 1 test-build dependency (github.com/bradleyjkemp/cupaloy), 1 runtime dependency (clang) and uses the in-built toolchain support to download the build dependency into the GOPATH.

Vendoring is useful to have reproducible builds, which in this case are less of a concern.
The new dependency I introduced golang.org/x/tools/go/ast/astutil is also part of the official Go tools, so there's less concern for any breaking changes.

So if you'd like @elliotchance, I'll remove the vendor folder?

@elliotchance
Copy link
Owner

Third party code should never be committed to the repository. You can use dep to lock down the versions you need (you may also have to update the .travis.yml).

Up until now there has been no dependencies (?) on the prod code so it hasn't been an issue.

@kamphaus kamphaus force-pushed the fix-693-switch-label-clause branch from 939dc05 to 0f6a60b Compare June 4, 2018 07:36
@kamphaus
Copy link
Contributor Author

kamphaus commented Jun 4, 2018

Vendor folder is removed.

@elliotchance
Copy link
Owner

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


transpiler/switch.go, line 13 at r5 (raw file):

	"github.com/elliotchance/c2go/program"
	"github.com/elliotchance/c2go/util"
	"golang.org/x/tools/go/ast/astutil"

Is this bundled with go or do you need it add to dep requirements?


Comments from Reviewable

@kamphaus
Copy link
Contributor Author

kamphaus commented Jun 5, 2018

transpiler/switch.go, line 13 at r5 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Is this bundled with go or do you need it add to dep requirements?

astutil is not bundled with go.

Installing/updating c2go with go get -u github.com/elliotchance/c2go will take care to download the latest version of astutil into the $GOPATH.

By needing to add it to dep requirements, did you mean add it to the documentation as a requirement, or did you mean to use the dep tool, which is a dependency management tool (https://golang.github.io/dep/docs/introduction.html)?

Since astutil is authored by Google, the chance of a breaking change should be minimal, so I don't see the need at the moment for a dependency management tool. (Maybe later, if there are other dependencies)

PS: see https://github.com/golang/dep/blob/master/docs/FAQ.md#should-i-commit-my-vendor-directory on the pros/cons of committing the vendor folder.


Comments from Reviewable

@elliotchance
Copy link
Owner

Review status: all files reviewed, 1 unresolved discussion (waiting on @elliotchance)


transpiler/switch.go, line 13 at r5 (raw file):

Previously, kamphaus wrote…

astutil is not bundled with go.

Installing/updating c2go with go get -u github.com/elliotchance/c2go will take care to download the latest version of astutil into the $GOPATH.

By needing to add it to dep requirements, did you mean add it to the documentation as a requirement, or did you mean to use the dep tool, which is a dependency management tool (https://golang.github.io/dep/docs/introduction.html)?

Since astutil is authored by Google, the chance of a breaking change should be minimal, so I don't see the need at the moment for a dependency management tool. (Maybe later, if there are other dependencies)

PS: see https://github.com/golang/dep/blob/master/docs/FAQ.md#should-i-commit-my-vendor-directory on the pros/cons of committing the vendor folder.

That's fair enough. c2go is still alpha enough that we can get away without locking versions.

As for committing the vendor folder, everything is a trade off. Unless your building extremely defensive software committing the vendor folder just bloats your codebase and opens the potential for people to tweak the vendor code.

I have only used dep for some projects at work, but have used package managers in other languages for years and I can't think of a single time in recently memory where a vendor has broken something that affected our builds or deploys.


Comments from Reviewable

@elliotchance elliotchance merged commit 9cd4973 into elliotchance:master Jun 12, 2018
@kamphaus kamphaus deleted the fix-693-switch-label-clause branch June 12, 2018 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants