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

Issue 134780043: code review 134780043: go.tools/cmd/vet: detect suspicious shifts

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 3 months ago by mjibson
Modified:
1 week, 5 days ago
Reviewers:
r, Éz, dsymonds, josharian
CC:
golang-codereviews, josharian, minux, dsymonds, dfc, axw, adg, r
Visibility:
Public.

Description

go.tools/cmd/vet: detect suspicious shifts

Patch Set 1 #

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

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

Total comments: 8

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

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

Total comments: 5

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

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

Total comments: 2

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

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

Total comments: 5

Patch Set 10 : diff -r 067839d7eb9b000c390a79f2f24a5b0e6975f574 https://code.google.com/p/go.tools #

Patch Set 11 : diff -r 067839d7eb9b000c390a79f2f24a5b0e6975f574 https://code.google.com/p/go.tools #

Total comments: 1

Patch Set 12 : diff -r 067839d7eb9b000c390a79f2f24a5b0e6975f574 https://code.google.com/p/go.tools #

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

Total comments: 1

Patch Set 14 : diff -r 067839d7eb9b000c390a79f2f24a5b0e6975f574 https://code.google.com/p/go.tools #

Patch Set 15 : diff -r 067839d7eb9b000c390a79f2f24a5b0e6975f574 https://code.google.com/p/go.tools #

Unified diffs Side-by-side diffs Delta from patch set Stats (+167 lines, -0 lines) Patch
M cmd/vet/doc.go View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
A cmd/vet/shift.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +83 lines, -0 lines 0 comments Download
A cmd/vet/testdata/shift.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +78 lines, -0 lines 0 comments Download

Messages

Total messages: 36
mjibson
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go.tools
8 years, 3 months ago (2014-08-24 20:52:10 UTC) #1
josharian
Thanks for doing this. Here's a first pass of comments, looking just at the implementation. ...
8 years, 3 months ago (2014-08-25 02:40:17 UTC) #2
minux
https://codereview.appspot.com/134780043/diff/40001/cmd/vet/shift.go File cmd/vet/shift.go (right): https://codereview.appspot.com/134780043/diff/40001/cmd/vet/shift.go#newcode68 cmd/vet/shift.go:68: size = 8 On 2014/08/25 02:40:17, josharian wrote: > ...
8 years, 3 months ago (2014-08-25 04:42:19 UTC) #3
dsymonds
https://codereview.appspot.com/134780043/diff/40001/cmd/vet/shift.go File cmd/vet/shift.go (right): https://codereview.appspot.com/134780043/diff/40001/cmd/vet/shift.go#newcode23 cmd/vet/shift.go:23: binaryExpr, join the next line onto this one. (keep ...
8 years, 3 months ago (2014-08-25 06:41:28 UTC) #4
mjibson
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, minux@golang.org, dsymonds@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
8 years, 3 months ago (2014-08-26 14:58:29 UTC) #5
josharian
Another round. Once you fix the panic from non-constant shifts, I'll run this over a ...
8 years, 3 months ago (2014-08-26 19:53:16 UTC) #6
josharian
> > The max meaningful shift for signed ints is one smaller than for unsigned ...
8 years, 3 months ago (2014-08-26 19:54:05 UTC) #7
mjibson
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, minux@golang.org, dsymonds@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
8 years, 3 months ago (2014-08-26 20:53:40 UTC) #8
josharian
Wow. This found 308 instances in the corpus, and based on a sampling, almost all ...
8 years, 3 months ago (2014-08-26 21:28:30 UTC) #9
dfc
+axw, Thanks Josh, very informative. Andrew, looks like there are a couple that need fixing ...
8 years, 3 months ago (2014-08-26 22:27:10 UTC) #10
axw
On 2014/08/26 22:27:10, dfc wrote: > +axw, > > Thanks Josh, very informative. > > ...
8 years, 3 months ago (2014-08-27 00:52:03 UTC) #11
josharian
> (Curiously, the corpus contains three out-of-date forks of llgo, but neither the > original ...
8 years, 3 months ago (2014-08-27 00:58:26 UTC) #12
axw
On 2014/08/27 00:58:26, josharian wrote: > > (Curiously, the corpus contains three out-of-date forks of ...
8 years, 3 months ago (2014-08-27 01:50:30 UTC) #13
minux
On Aug 26, 2014 5:28 PM, <josharian@gmail.com> wrote: > > Wow. This found 308 instances ...
8 years, 3 months ago (2014-08-27 06:05:57 UTC) #14
adg
s/suspicious/shifty/g
8 years, 3 months ago (2014-08-27 12:36:09 UTC) #15
josharian
> _W is at most 64 and t is uint64, so why govet gives the ...
8 years, 3 months ago (2014-08-27 22:00:28 UTC) #16
josharian
LGTM I'll wait another day or two before submitting. Thanks again for doing this.
8 years, 3 months ago (2014-08-27 22:01:24 UTC) #17
minux
On Aug 27, 2014 6:00 PM, <josharian@gmail.com> wrote: >> >> _W is at most 64 ...
8 years, 3 months ago (2014-08-27 22:12:31 UTC) #18
mjibson
On Wed, Aug 27, 2014 at 6:12 PM, minux <minux@golang.org> wrote: > > could we ...
8 years, 3 months ago (2014-08-27 22:18:56 UTC) #19
minux
On Aug 27, 2014 6:18 PM, "Matt Jibson" <matt.jibson@gmail.com> wrote: > On Wed, Aug 27, ...
8 years, 3 months ago (2014-08-27 23:44:41 UTC) #20
dsymonds
Rob should give this a review before anyone submits it. https://codereview.appspot.com/134780043/diff/120001/cmd/vet/shift.go File cmd/vet/shift.go (right): https://codereview.appspot.com/134780043/diff/120001/cmd/vet/shift.go#newcode44 ...
8 years, 3 months ago (2014-08-27 23:56:39 UTC) #21
josharian
> could we check for >= size for left shifts and > size for right ...
8 years, 3 months ago (2014-08-28 00:27:46 UTC) #22
mjibson
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, minux@golang.org, dsymonds@golang.org, dave@cheney.net, axwalk@gmail.com, adg@golang.org, r@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
8 years, 3 months ago (2014-08-28 18:02:40 UTC) #23
dsymonds
LGTM The ball's in Rob's court.
8 years, 3 months ago (2014-08-29 00:57:42 UTC) #24
r
https://codereview.appspot.com/134780043/diff/180001/cmd/vet/doc.go File cmd/vet/doc.go (right): https://codereview.appspot.com/134780043/diff/180001/cmd/vet/doc.go#newcode154 cmd/vet/doc.go:154: Suspicious shifts s/uspicious s// https://codereview.appspot.com/134780043/diff/180001/cmd/vet/shift.go File cmd/vet/shift.go (right): https://codereview.appspot.com/134780043/diff/180001/cmd/vet/shift.go#newcode21 ...
8 years, 3 months ago (2014-08-29 02:09:21 UTC) #25
mjibson
https://codereview.appspot.com/134780043/diff/180001/cmd/vet/shift.go File cmd/vet/shift.go (right): https://codereview.appspot.com/134780043/diff/180001/cmd/vet/shift.go#newcode71 cmd/vet/shift.go:71: default: On 2014/08/29 02:09:21, r wrote: > what about ...
8 years, 3 months ago (2014-08-29 17:06:58 UTC) #26
josharian
On 2014/08/29 17:06:58, mjibson wrote: > https://codereview.appspot.com/134780043/diff/180001/cmd/vet/shift.go > File cmd/vet/shift.go (right): > > https://codereview.appspot.com/134780043/diff/180001/cmd/vet/shift.go#newcode71 > ...
8 years, 3 months ago (2014-08-29 17:16:31 UTC) #27
r
https://codereview.appspot.com/134780043/diff/180001/cmd/vet/shift.go File cmd/vet/shift.go (right): https://codereview.appspot.com/134780043/diff/180001/cmd/vet/shift.go#newcode71 cmd/vet/shift.go:71: default: int and uint can be as small as ...
8 years, 3 months ago (2014-08-29 17:17:15 UTC) #28
mjibson
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, minux@golang.org, dsymonds@golang.org, dave@cheney.net, axwalk@gmail.com, adg@golang.org, r@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
8 years, 3 months ago (2014-08-29 17:29:59 UTC) #29
r
https://codereview.appspot.com/134780043/diff/220001/cmd/vet/shift.go File cmd/vet/shift.go (right): https://codereview.appspot.com/134780043/diff/220001/cmd/vet/shift.go#newcode72 cmd/vet/shift.go:72: size = 32 // These types may be as ...
8 years, 3 months ago (2014-08-29 17:42:57 UTC) #30
mjibson
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, minux@golang.org, dsymonds@golang.org, dave@cheney.net, axwalk@gmail.com, adg@golang.org, r@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
8 years, 3 months ago (2014-08-29 17:51:42 UTC) #31
r
https://codereview.appspot.com/134780043/diff/250001/cmd/vet/shift.go File cmd/vet/shift.go (right): https://codereview.appspot.com/134780043/diff/250001/cmd/vet/shift.go#newcode79 cmd/vet/shift.go:79: f.Badf(node.Pos(), "%s has size %d but is shifted by ...
8 years, 3 months ago (2014-08-29 18:00:13 UTC) #32
mjibson
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, minux@golang.org, dsymonds@golang.org, dave@cheney.net, axwalk@gmail.com, adg@golang.org, r@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
8 years, 3 months ago (2014-08-29 18:12:28 UTC) #33
r
LGTM
8 years, 3 months ago (2014-08-29 18:15:51 UTC) #34
r
*** Submitted as https://code.google.com/p/go/source/detail?r=37b8aca2b320&repo=tools *** go.tools/cmd/vet: detect suspicious shifts LGTM=josharian, dsymonds, r R=golang-codereviews, josharian, minux, ...
8 years, 3 months ago (2014-08-29 18:17:04 UTC) #35
Éz
1 week, 5 days ago (2022-11-18 18:54:27 UTC) #36
On 2014/08/29 18:17:04, r wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=37b8aca2b320&repo=tools ***
> 
> go.tools/cmd/vet: detect suspicious shifts
> 
> LGTM=josharian, dsymonds, r
> R=golang-codereviews, josharian, minux, dsymonds, dave, axwalk, adg, r
> CC=golang-codereviews
> https://codereview.appspot.com/134780043
> 
> Committer: Rob Pike <mailto:r@golang.org>

Apply repo
Sign in to reply to this message.

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