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

Issue 163740043: code review 163740043: go/types: don't mark external package objects as used (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 5 months ago by gri
Modified:
9 years, 4 months ago
Reviewers:
adonovan
CC:
adonovan, golang-codereviews
Visibility:
Public.

Description

go/types: don't mark external package objects as used Also removes a potential race condition regarding the used flag of Var objects when type-checking packages concurrently. Implementation: Rather than marking all used dot-imported objects and then deduce which corresponding package was used, now we consider all dot-imported packages as unused and remove each package from the unused packages map as objects are used. Now only objects that can be marked as used have a used field (variables, labels, and packages). As a result, the code became cleaner and simpler. Fixes issue 8969.

Patch Set 1 #

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

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

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

Total comments: 1

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

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

Total comments: 2

Patch Set 7 : diff -r 8688baf0f657d93c243caec3e9b91ca211d47027 https://code.google.com/p/go.tools #

Unified diffs Side-by-side diffs Delta from patch set Stats (+105 lines, -87 lines) Patch
M go/types/assignments.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M go/types/call.go View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M go/types/check.go View 1 2 3 4 5 6 chunks +21 lines, -18 lines 0 comments Download
M go/types/check_test.go View 1 1 chunk +1 line, -0 lines 0 comments Download
M go/types/object.go View 1 8 chunks +13 lines, -20 lines 0 comments Download
M go/types/resolver.go View 1 2 3 4 6 chunks +25 lines, -29 lines 0 comments Download
M go/types/return.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M go/types/scope.go View 1 2 3 4 5 6 1 chunk +9 lines, -4 lines 0 comments Download
M go/types/stmt.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
A go/types/testdata/importdecl1a.src View 1 1 chunk +11 lines, -0 lines 0 comments Download
A go/types/testdata/importdecl1b.src View 1 1 chunk +7 lines, -0 lines 0 comments Download
M go/types/typexpr.go View 1 2 3 4 5 chunks +12 lines, -12 lines 0 comments Download

Messages

Total messages: 10
gri
Hello adonovan@google.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.tools
9 years, 5 months ago (2014-10-22 23:43:12 UTC) #1
adonovan
LGTM Great! This is much better. Sorry for not spotting the original problem, which is ...
9 years, 5 months ago (2014-10-23 00:27:35 UTC) #2
gri
Well, the ssa tests fail, sigh. I think I know what's wrong. Regarding the comment: ...
9 years, 5 months ago (2014-10-23 03:26:38 UTC) #3
gri
PTAL This time use the correct scope when removing a dot-imported object's package from the ...
9 years, 5 months ago (2014-10-23 04:44:22 UTC) #4
adonovan
On 2014/10/23 04:44:22, gri wrote: > PTAL > > This time use the correct scope ...
9 years, 5 months ago (2014-10-23 05:03:40 UTC) #5
adonovan
https://codereview.appspot.com/163740043/diff/90001/go/types/scope.go File go/types/scope.go (right): https://codereview.appspot.com/163740043/diff/90001/go/types/scope.go#newcode74 go/types/scope.go:74: // returns that scope and object. If no such ...
9 years, 5 months ago (2014-10-23 05:03:46 UTC) #6
gri
There's no API change that's visible. https://codereview.appspot.com/163740043/diff/90001/go/types/scope.go File go/types/scope.go (right): https://codereview.appspot.com/163740043/diff/90001/go/types/scope.go#newcode74 go/types/scope.go:74: // returns that ...
9 years, 5 months ago (2014-10-23 16:36:15 UTC) #7
adonovan
I meant to (*Scope).LookupParent, not the Objects themselves. On 23 October 2014 12:36, <gri@golang.org> wrote: ...
9 years, 5 months ago (2014-10-23 16:38:48 UTC) #8
gri
*** Submitted as https://code.google.com/p/go/source/detail?r=5ebd1091b374&repo=tools *** go/types: don't mark external package objects as used Also removes ...
9 years, 5 months ago (2014-10-23 16:39:24 UTC) #9
gri
9 years, 5 months ago (2014-10-23 16:43:46 UTC) #10
Ah, good point. There may be.

I was contemplating adding another internal function, but then didn't do
it. Let's see.
- gri

On Thu, Oct 23, 2014 at 9:38 AM, Alan Donovan <adonovan@google.com> wrote:

> I meant to (*Scope).LookupParent, not the Objects themselves.
>
> On 23 October 2014 12:36, <gri@golang.org> wrote:
>
>> There's no API change that's visible.
>>
>>
>>
>>
>> https://codereview.appspot.com/163740043/diff/90001/go/types/scope.go
>> File go/types/scope.go (right):
>>
>> https://codereview.appspot.com/163740043/diff/90001/go/
>> types/scope.go#newcode74
>> go/types/scope.go:74: // returns that scope and object. If no such scope
>> exists, the result is (nil, nil).
>> On 2014/10/23 05:03:45, adonovan wrote:
>>
>>> Presumably the scope and obj.Parent() can only differ because of dot
>>>
>> imports.
>>
>>> Might be worth a mention.
>>>
>>
>> Done.
>>
>> https://codereview.appspot.com/163740043/
>>
>
>
Sign in to reply to this message.

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