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

Issue 97480043: code review 97480043: go.tools/cmd/vet: add a check for finalizers that refer...

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 10 months ago by robryk
Modified:
9 years, 3 months ago
Reviewers:
josharian
CC:
golang-codereviews, r, gobot, josharian
Visibility:
Public.

Description

go.tools/cmd/vet: add a check for finalizers that reference their object This check guards against an accidental direct reference to the object being finalized in the finalizer, instead of using the finalizer's argument. This check, as implemented currently, checks only finalizers specified as function literals. Fixes issue 7546.

Patch Set 1 #

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

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

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

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

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

Total comments: 10

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

Total comments: 2

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

Total comments: 2

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

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+98 lines, -0 lines) Patch
M cmd/vet/doc.go View 1 2 3 4 5 6 7 8 1 chunk +7 lines, -0 lines 1 comment Download
A cmd/vet/finalizers.go View 1 2 3 4 5 6 7 1 chunk +65 lines, -0 lines 3 comments Download
A cmd/vet/testdata/finalizers.go View 1 2 3 4 5 6 1 chunk +26 lines, -0 lines 1 comment Download

Messages

Total messages: 19
robryk
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go.tools
9 years, 10 months ago (2014-05-14 16:44:08 UTC) #1
r
We are in feature freeze pending the Go 1.3 release. Please send this again once ...
9 years, 10 months ago (2014-05-14 17:33:24 UTC) #2
gobot
R=golang-dev (assigned by r@golang.org)
9 years, 10 months ago (2014-05-14 17:33:44 UTC) #3
r
Please sync your client and update your changes to the new registration mechanism used by ...
9 years, 9 months ago (2014-06-13 17:37:14 UTC) #4
gobot
R=r@golang.org (assigned by r@golang.org)
9 years, 9 months ago (2014-06-16 19:50:30 UTC) #5
robryk
On 2014/06/13 17:37:14, r wrote: > Please sync your client and update your changes to ...
9 years, 9 months ago (2014-06-18 17:18:54 UTC) #6
josharian
As a data point, I ran this over a fairly large corpus of public code. ...
9 years, 9 months ago (2014-06-19 18:55:15 UTC) #7
r
And how many finalizers appear in that same corpus? I think this is a rare ...
9 years, 9 months ago (2014-06-19 19:59:59 UTC) #8
josharian
> And how many finalizers appear in that same corpus? 1152 > I think this ...
9 years, 9 months ago (2014-06-19 20:15:14 UTC) #9
josharian
https://codereview.appspot.com/97480043/diff/100001/cmd/vet/finalizers.go File cmd/vet/finalizers.go (right): https://codereview.appspot.com/97480043/diff/100001/cmd/vet/finalizers.go#newcode1 cmd/vet/finalizers.go:1: // Copyright 2013 The Go Authors. All rights reserved. ...
9 years, 9 months ago (2014-06-19 20:17:08 UTC) #10
robryk
https://codereview.appspot.com/97480043/diff/100001/cmd/vet/finalizers.go File cmd/vet/finalizers.go (right): https://codereview.appspot.com/97480043/diff/100001/cmd/vet/finalizers.go#newcode1 cmd/vet/finalizers.go:1: // Copyright 2013 The Go Authors. All rights reserved. ...
9 years, 9 months ago (2014-06-19 21:38:49 UTC) #11
josharian
The code looks good. Want to add an entry to doc.go? Sorry for missing these ...
9 years, 9 months ago (2014-06-19 22:04:47 UTC) #12
robryk
Added an entry in doc.go, too. https://codereview.appspot.com/97480043/diff/120001/cmd/vet/finalizers.go File cmd/vet/finalizers.go (right): https://codereview.appspot.com/97480043/diff/120001/cmd/vet/finalizers.go#newcode13 cmd/vet/finalizers.go:13: register("finalizers", "check that ...
9 years, 9 months ago (2014-06-19 22:11:40 UTC) #13
josharian
LGTM but I'll wait for r before submitting One wording tweak before r looks... https://codereview.appspot.com/97480043/diff/70003/cmd/vet/doc.go ...
9 years, 9 months ago (2014-06-19 22:27:13 UTC) #14
robryk
https://codereview.appspot.com/97480043/diff/70003/cmd/vet/doc.go File cmd/vet/doc.go (right): https://codereview.appspot.com/97480043/diff/70003/cmd/vet/doc.go#newcode152 cmd/vet/doc.go:152: finalize. This causes the object to never be unreferenced ...
9 years, 9 months ago (2014-06-19 22:29:02 UTC) #15
r
https://codereview.appspot.com/97480043/diff/150001/cmd/vet/doc.go File cmd/vet/doc.go (right): https://codereview.appspot.com/97480043/diff/150001/cmd/vet/doc.go#newcode151 cmd/vet/doc.go:151: Finalizer functions that refer directly to the objects that ...
9 years, 9 months ago (2014-06-19 22:35:39 UTC) #16
josharian
Ping, robryk.
9 years, 9 months ago (2014-07-03 23:39:32 UTC) #17
josharian
Ping, robryk. Any interest in, er, finalizing this? If not, mind if I finish this ...
9 years, 7 months ago (2014-08-12 18:22:12 UTC) #18
gobot
9 years, 3 months ago (2014-12-19 05:19:28 UTC) #19
R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/97480043 is best) in the description in your
new CL.

Thanks very much.
Sign in to reply to this message.

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