Skip to content
This repository was archived by the owner on May 11, 2020. It is now read-only.

Expose access to Ops to avoid stringly typing downstream - #172

Open
silasdavis wants to merge 1 commit into
go-interpreter:masterfrom
silasdavis:getop
Open

Expose access to Ops to avoid stringly typing downstream#172
silasdavis wants to merge 1 commit into
go-interpreter:masterfrom
silasdavis:getop

Conversation

@silasdavis

@silasdavis silasdavis commented Oct 9, 2019

Copy link
Copy Markdown

Avoid this kind of sadness:

https://github.com/perlin-network/life/blob/master/compiler/ssa.go#L445-L454

with:

operators.Get(operators.CurrentMemory).Name

Signed-off-by: Silas Davis silas@monax.io

@silasdavis
silasdavis force-pushed the getop branch 3 times, most recently from 3532253 to b84fb2b Compare October 9, 2019 19:08
@codecov-io

codecov-io commented Oct 9, 2019

Copy link
Copy Markdown

Codecov Report

Merging #172 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
+ Coverage   69.71%   69.72%   +0.01%     
==========================================
  Files          48       48              
  Lines        5484     5486       +2     
==========================================
+ Hits         3823     3825       +2     
  Misses       1317     1317              
  Partials      344      344
Impacted Files Coverage Δ
wasm/operators/op.go 76.47% <100%> (+1.47%) ⬆️

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 01d3d33...5f0a572. Read the comment docs.

Signed-off-by: Silas Davis <silas@monax.io>
Comment thread wasm/operators/op.go
return op, nil
}

func Get(opcode byte) Op {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a doc string for this?

@iwasaki-kenta

Copy link
Copy Markdown

Might using Stringer for op's be a better option over directly fetching the name of the op?

https://github.com/golang/tools/blob/master/cmd/stringer/stringer.go

@twitchyliquid64

Copy link
Copy Markdown
Contributor

Feel free to integrate stringer for Op if you wish.

Apologies for the confusion, what I meant was to add a doc comment to Get(opcode byte) Op. Its part of the exported API surface, so we need a doc comment so it shows up with documentation in godoc (See the Commentary --> Doc comments section in effective Go for more rationale).

@sbinet

sbinet commented Nov 4, 2019

Copy link
Copy Markdown
Contributor

ping?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants