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

Issue 12552047: code review 12552047: go.tools/go.types: retain ast.Node links on demand only (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 2 months ago by gri
Modified:
12 years, 1 month ago
Reviewers:
r, adonovan, rocky
CC:
r, golang-dev
Visibility:
Public.

Description

go.tools/go.types: retain ast.Node links on demand only - support Info.Scopes mapping that maps ast.Nodes to the respective *Scope - remove old node link from *Scope - added corresponding API test Also: re-enable debug mode (the faster version was only important for the go api tool, which has its own version now).

Patch Set 1 #

Patch Set 2 : diff -r 6698ca2900e2 https://code.google.com/p/go.tools #

Patch Set 3 : diff -r 6698ca2900e2 https://code.google.com/p/go.tools #

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

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+160 lines, -38 lines) Patch
M go/types/api.go View 1 2 chunks +21 lines, -1 line 0 comments Download
M go/types/api_test.go View 1 3 chunks +127 lines, -1 line 3 comments Download
M go/types/check.go View 1 2 chunks +8 lines, -7 lines 0 comments Download
M go/types/resolver.go View 1 1 chunk +1 line, -3 lines 0 comments Download
M go/types/scope.go View 1 3 chunks +0 lines, -20 lines 0 comments Download
M go/types/stmt.go View 1 1 chunk +1 line, -3 lines 0 comments Download
M go/types/typexpr.go View 1 1 chunk +2 lines, -3 lines 0 comments Download

Messages

Total messages: 18
gri
Hello adonovan@google.com, r@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.tools
12 years, 2 months ago (2013-08-09 01:40:11 UTC) #1
r
LGTM
12 years, 2 months ago (2013-08-09 01:44:10 UTC) #2
gri
*** Submitted as https://code.google.com/p/go/source/detail?r=15ba0a163203&repo=tools *** go.tools/go.types: retain ast.Node links on demand only - support Info.Scopes ...
12 years, 2 months ago (2013-08-09 03:27:34 UTC) #3
rocky
What changes (if any) need to be made in the ssa-interpreter need to allow it ...
12 years, 2 months ago (2013-08-10 00:53:21 UTC) #4
gri
No changes are needed I think. ssa didn't use that information in the first place. ...
12 years, 2 months ago (2013-08-10 00:57:49 UTC) #5
rocky
Yes, sure, the ssa interpreter doesn't use that information, but the fork of it on ...
12 years, 2 months ago (2013-08-10 01:33:18 UTC) #6
gri
It's trivial to add the information if you need it. The importer allocates all maps ...
12 years, 2 months ago (2013-08-10 04:18:02 UTC) #7
rocky
On 2013/08/10 04:18:02, gri wrote: > It's trivial to add the information if you need ...
12 years, 2 months ago (2013-08-10 04:40:03 UTC) #8
gri
go/types is very close to functionally complete. There are known bugs (issue tracker, and TODOs ...
12 years, 2 months ago (2013-08-10 04:52:18 UTC) #9
rocky
On 2013/08/10 04:52:18, gri wrote: > go/types is very close to functionally complete. There are ...
12 years, 2 months ago (2013-08-10 06:15:09 UTC) #10
rocky
I've been looking again to look at the bugs and TODO's as you mentioned previously. ...
12 years, 2 months ago (2013-08-11 03:45:24 UTC) #11
gri
The Info struct can be populated with maps that provide you access to the type ...
12 years, 2 months ago (2013-08-11 04:34:56 UTC) #12
rocky
On Sun, Aug 11, 2013 at 12:34 AM, Robert Griesemer <gri@golang.org> wrote: > The Info ...
12 years, 2 months ago (2013-08-11 17:17:39 UTC) #13
gri
go.types.Eval does exactly what you are asking for and has been there for a while ...
12 years, 1 month ago (2013-08-13 21:20:33 UTC) #14
rocky
I had previously tried go.types.Eval, but it has a number of problems. The most serious ...
12 years, 1 month ago (2013-08-14 02:18:42 UTC) #15
gri
Let me say it one more time: The issues you have with a debugger or ...
12 years, 1 month ago (2013-08-14 04:00:53 UTC) #16
adonovan
LGTM https://codereview.appspot.com/12552047/diff/2/go/types/api_test.go File go/types/api_test.go (right): https://codereview.appspot.com/12552047/diff/2/go/types/api_test.go#newcode89 go/types/api_test.go:89: func TestScopesInfo(t *testing.T) { Nice test. https://codereview.appspot.com/12552047/diff/2/go/types/api_test.go#newcode198 go/types/api_test.go:198: ...
12 years, 1 month ago (2013-08-14 17:05:11 UTC) #17
gri
12 years, 1 month ago (2013-08-14 17:45:31 UTC) #18
Message was sent while issue was closed.
https://codereview.appspot.com/12552047/diff/2/go/types/api_test.go
File go/types/api_test.go (right):

https://codereview.appspot.com/12552047/diff/2/go/types/api_test.go#newcode198
go/types/api_test.go:198: // look for matching scope description
On 2013/08/14 17:05:12, adonovan wrote:
> Would it also be useful to test the scope tree topology, i.e. Parent() links?

Yes, but I left it out because I think it might be done elsewhere - in a
sanity-checking phase that runs through the data structures generated after a
type check.
Sign in to reply to this message.

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