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

Delta Between Two Patch Sets: cmd/vet/shift.go

Issue 134780043: code review 134780043: go.tools/cmd/vet: detect suspicious shifts
Left Patch Set: diff -r 8a11e1300048655497f877f9408b0b1ac47886c0 https://code.google.com/p/go.tools Created 9 years, 7 months ago
Right Patch Set: diff -r 067839d7eb9b000c390a79f2f24a5b0e6975f574 https://code.google.com/p/go.tools Created 9 years, 7 months ago
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments. Please Sign in to add in-line comments.
Jump to:
Left: Side by side diff | Download
Right: Side by side diff | Download
« no previous file with change/comment | « cmd/vet/doc.go ('k') | cmd/vet/testdata/shift.go » ('j') | no next file with change/comment »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
LEFTRIGHT
1 // Copyright 2014 The Go Authors. All rights reserved. 1 // Copyright 2014 The Go Authors. All rights reserved.
2 // Use of this source code is governed by a BSD-style 2 // Use of this source code is governed by a BSD-style
3 // license that can be found in the LICENSE file. 3 // license that can be found in the LICENSE file.
4 4
5 /* 5 /*
6 This file contains the code to check for suspicious shifts. 6 This file contains the code to check for suspicious shifts.
7 */ 7 */
8 8
9 package main 9 package main
10 10
11 import ( 11 import (
12 "go/ast" 12 "go/ast"
13 "go/token" 13 "go/token"
14 "strconv"
15 14
15 "code.google.com/p/go.tools/go/exact"
16 "code.google.com/p/go.tools/go/types" 16 "code.google.com/p/go.tools/go/types"
17 ) 17 )
18 18
19 func init() { 19 func init() {
20 register("shift", 20 register("shift",
21 » » "check for suspicious shifts", 21 » » "check for useless shifts",
22 checkShift, 22 checkShift,
23 » » binaryExpr, 23 » » binaryExpr, assignStmt)
dsymonds 2014/08/25 06:41:28 join the next line onto this one. (keep the list o
24 » » assignStmt)
25 } 24 }
26 25
27 func checkShift(f *File, node ast.Node) { 26 func checkShift(f *File, node ast.Node) {
28 switch node := node.(type) { 27 switch node := node.(type) {
29 case *ast.BinaryExpr: 28 case *ast.BinaryExpr:
30 if node.Op == token.SHL || node.Op == token.SHR { 29 if node.Op == token.SHL || node.Op == token.SHR {
31 » » » checkSuspiciousShift(f, node, node.X, node.Y, node.Op) 30 » » » checkLongShift(f, node, node.X, node.Y)
32 } 31 }
33 case *ast.AssignStmt: 32 case *ast.AssignStmt:
34 » » for i, lhs := range node.Lhs { 33 » » if len(node.Lhs) != 1 || len(node.Rhs) != 1 {
josharian 2014/08/25 02:40:17 From the spec: "In assignment operations, both the
35 » » » if len(node.Rhs) <= i { 34 » » » return
36 » » » » break 35 » » }
37 » » » } 36 » » if node.Tok == token.SHL_ASSIGN || node.Tok == token.SHR_ASSIGN {
38 » » » if node.Tok == token.SHL_ASSIGN || node.Tok == token.SHR _ASSIGN { 37 » » » checkLongShift(f, node, node.Lhs[0], node.Rhs[0])
39 » » » » rhs := node.Rhs[i]
40 » » » » checkSuspiciousShift(f, node, lhs, rhs, node.Tok )
41 » » » }
42 } 38 }
43 } 39 }
44 } 40 }
45 41
46 // checkSuspiciousShift checks if shift or shift-assign operations shift by more 42 // checkLongShift checks if shift or shift-assign operations shift by more than
47 // than the length of the underlying variable. 43 // the length of the underlying variable.
48 func checkSuspiciousShift(f *File, node ast.Node, x, y ast.Expr, op token.Token) { 44 func checkLongShift(f *File, node ast.Node, x, y ast.Expr) {
josharian 2014/08/25 02:40:17 op is unused.
49 » basic, ok := y.(*ast.BasicLit) 45 » v := f.pkg.types[y].Value
josharian 2014/08/25 02:40:17 Use f.pkg.types[y].Value to get a constant value i
50 » if !ok || basic.Kind != token.INT { 46 » if v == nil {
51 return 47 return
52 } 48 }
53 » amt, err := strconv.Atoi(basic.Value) 49 » amt, ok := exact.Int64Val(v)
54 » if err != nil { 50 » if !ok {
55 return 51 return
56 } 52 }
57 t := f.pkg.types[x].Type 53 t := f.pkg.types[x].Type
58 » if t != nil { 54 » if t == nil {
59 » » t = t.Underlying() 55 » » return
60 } 56 }
61 » b, ok := t.(*types.Basic) 57 » b, ok := t.Underlying().(*types.Basic)
62 if !ok { 58 if !ok {
63 return 59 return
64 } 60 }
65 » size := 0 61 » var size int64
62 » var msg string
66 switch b.Kind() { 63 switch b.Kind() {
67 case types.Uint8, types.Int8: 64 case types.Uint8, types.Int8:
68 size = 8 65 size = 8
josharian 2014/08/25 02:40:17 The max meaningful shift for signed ints is one sm
minux 2014/08/25 04:42:18 this is not right. int8 >> 8 is still meaningful,
69 case types.Uint16, types.Int16: 66 case types.Uint16, types.Int16:
70 size = 16 67 size = 16
71 case types.Uint32, types.Int32: 68 case types.Uint32, types.Int32:
72 size = 32 69 size = 32
73 case types.Uint64, types.Int64: 70 case types.Uint64, types.Int64:
74 size = 64 71 size = 64
72 case types.Int, types.Uint, types.Uintptr:
73 // These types may be as small as 32 bits, but no smaller.
74 size = 32
75 msg = "might be "
75 default: 76 default:
76 return 77 return
77 } 78 }
78 if amt >= size { 79 if amt >= size {
79 ident := f.gofmt(x) 80 ident := f.gofmt(x)
80 » » f.Badf(node.Pos(), "suspicious shift of %s", ident) 81 » » f.Badf(node.Pos(), "%s %stoo small for shift of %d", ident, msg, amt)
dsymonds 2014/08/25 06:41:28 I'm not sure what one would do with such a message
81 } 82 }
82 } 83 }
LEFTRIGHT

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