Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(119)

Issue 7071058: code review 7071058: exp/ssa: API and documentation. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 9 months ago by adonovan
Modified:
8 years, 9 months ago
Reviewers:
iant
CC:
gri1, iant2, crawshaw1, bradfitz, gri, iant, golang-dev
Visibility:
Public.

Description

exp/ssa: API and documentation.

Patch Set 1 #

Patch Set 2 : diff -r 1399878c6731 https://code.google.com/p/go/ #

Total comments: 28

Patch Set 3 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #

Patch Set 4 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #

Patch Set 5 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #

Patch Set 6 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #

Total comments: 79

Patch Set 7 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #

Total comments: 39

Patch Set 8 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #

Patch Set 9 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #

Patch Set 10 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #

Patch Set 11 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #

Total comments: 26

Patch Set 12 : diff -r 76dc0d7cec07 https://code.google.com/p/go/ #

Patch Set 13 : diff -r fd51d6123dfa https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1234 lines, -0 lines) Patch
A src/pkg/exp/ssa/doc.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +113 lines, -0 lines 0 comments Download
A src/pkg/exp/ssa/ssa.go View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1121 lines, -0 lines 0 comments Download

Messages

Total messages: 20
gri
some superficial comments - need to spend more time on this later (not today) - ...
8 years, 9 months ago (2013-01-09 22:55:00 UTC) #1
adonovan
Great comments---thanks. All 'done' as suggested except where noted. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newcode14 src/pkg/exp/ssa/ssa.go:14: ...
8 years, 9 months ago (2013-01-10 15:22:56 UTC) #2
adonovan
Hello gri@google.com, iant@google.com, golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
8 years, 9 months ago (2013-01-15 22:50:18 UTC) #3
crawshaw1
Minor comments. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/doc.go File src/pkg/exp/ssa/doc.go (right): https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/doc.go#newcode1 src/pkg/exp/ssa/doc.go:1: // exp/ssa: SSA representation of Go programs ...
8 years, 9 months ago (2013-01-17 16:08:48 UTC) #4
adonovan
Thanks, all good comments. All done, except the Builder/builder merge; will wait for gri's thoughts. ...
8 years, 9 months ago (2013-01-17 17:26:28 UTC) #5
adonovan
Hello gri@google.com, iant@google.com, crawshaw@google.com (cc: golang-dev@googlegroups.com), Please take another look.
8 years, 9 months ago (2013-01-17 17:29:50 UTC) #6
bradfitz
https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go File src/pkg/exp/ssa/doc.go (right): https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go#newcode1 src/pkg/exp/ssa/doc.go:1: // exp/ssa: SSA representation of Go programs drop this ...
8 years, 9 months ago (2013-01-17 19:12:50 UTC) #7
adonovan
https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go File src/pkg/exp/ssa/doc.go (right): https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go#newcode1 src/pkg/exp/ssa/doc.go:1: // exp/ssa: SSA representation of Go programs On 2013/01/17 ...
8 years, 9 months ago (2013-01-17 21:41:29 UTC) #8
gri
First round. More to come. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newcode36 src/pkg/exp/ssa/ssa.go:36: type Builder interface { ...
8 years, 9 months ago (2013-01-18 00:07:54 UTC) #9
gri
Some more comments. I feel that the API is drowned out by documentation. I don't ...
8 years, 9 months ago (2013-01-18 01:58:14 UTC) #10
adonovan
Thanks for all the comments; responses inline. PTAL. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newcode36 src/pkg/exp/ssa/ssa.go:36: type ...
8 years, 9 months ago (2013-01-22 04:37:54 UTC) #11
adonovan
I've implemented the changed to Phi/Jump/If we discussed; it's much cleaner: more compact and with ...
8 years, 9 months ago (2013-01-22 17:37:33 UTC) #12
adonovan
Updates since last snapshot: - new type Program: this is the toplevel container populated by ...
8 years, 9 months ago (2013-01-23 20:44:21 UTC) #13
iant
FYI https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newcode33 src/pkg/exp/ssa/ssa.go:33: // A Package a single analyzed Go package, ...
8 years, 9 months ago (2013-01-23 23:41:41 UTC) #14
adonovan
Thanks for the comments. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newcode33 src/pkg/exp/ssa/ssa.go:33: // A Package a single ...
8 years, 9 months ago (2013-01-24 02:47:54 UTC) #15
adonovan
Deltas since last review: - abandoned the multigraph formulation of the CFG; it's too hard ...
8 years, 9 months ago (2013-01-24 17:07:51 UTC) #16
iant
https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newcode60 src/pkg/exp/ssa/ssa.go:60: ImplementsMember() // dummy method to indicate the "implements" relation. ...
8 years, 9 months ago (2013-01-24 18:05:54 UTC) #17
adonovan
https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newcode60 src/pkg/exp/ssa/ssa.go:60: ImplementsMember() // dummy method to indicate the "implements" relation. ...
8 years, 9 months ago (2013-01-24 19:29:27 UTC) #18
iant
LGTM
8 years, 9 months ago (2013-01-24 21:17:33 UTC) #19
adonovan
8 years, 9 months ago (2013-01-24 22:21:52 UTC) #20
*** Submitted as https://code.google.com/p/go/source/detail?r=3ef1c4fabb5f ***

exp/ssa: API and documentation.

R=gri, iant, crawshaw, bradfitz, gri, iant
CC=golang-dev
https://codereview.appspot.com/7071058
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b