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

Expose access to Ops to avoid stringly typing downstream #172

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

silasdavis
Copy link

@silasdavis silasdavis commented Oct 9, 2019

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 [email protected]

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

codecov-io commented Oct 9, 2019

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.

@@ -85,3 +85,7 @@ func New(code byte) (Op, error) {
}
return op, nil
}

func Get(opcode byte) Op {
Copy link
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

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
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
Copy link
Contributor

sbinet commented Nov 4, 2019

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