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

Issue 7097048: code review 7097048: cmd/vet: detect misuse of atomic.Add* (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 2 months ago by divoxx
Modified:
7 years, 1 month ago
Reviewers:
CC:
dvyukov, golang-dev, remyoudompheng, rsc
Visibility:
Public.

Description

cmd/vet: detect misuse of atomic.Add* Re-assigning the return value of an atomic operation to the same variable being operated is a common mistake: x = atomic.AddUint64(&x, 1) Add this check to go vet. Fixes issue 4065.

Patch Set 1 #

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

Total comments: 1

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

Total comments: 2

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

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

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

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

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

Total comments: 2

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

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

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

Total comments: 7

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+107 lines, -1 line) Patch
M src/cmd/vet/Makefile View 1 1 chunk +1 line, -1 line 0 comments Download
A src/cmd/vet/atomic.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +90 lines, -0 lines 0 comments Download
M src/cmd/vet/main.go View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +16 lines, -0 lines 0 comments Download

Messages

Total messages: 21
dvyukov
https://codereview.appspot.com/7097048/diff/2001/src/cmd/vet/main.go File src/cmd/vet/main.go (right): https://codereview.appspot.com/7097048/diff/2001/src/cmd/vet/main.go#newcode210 src/cmd/vet/main.go:210: // walkCall walks a call expression. comment is wrong
7 years, 2 months ago (2013-01-12 11:24:32 UTC) #1
divoxx
Hello dvyukov@google.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
7 years, 2 months ago (2013-01-12 11:24:40 UTC) #2
dvyukov
On 2013/01/12 11:24:40, divoxx wrote: > Hello mailto:dvyukov@google.com (cc: mailto:golang-dev@googlegroups.com), > > I'd like you ...
7 years, 2 months ago (2013-01-12 11:28:08 UTC) #3
remyoudompheng
https://codereview.appspot.com/7097048/diff/6001/src/cmd/vet/atomic.go File src/cmd/vet/atomic.go (right): https://codereview.appspot.com/7097048/diff/6001/src/cmd/vet/atomic.go#newcode20 src/cmd/vet/atomic.go:20: if call, ok := rexp.(*ast.CallExpr); ok { the indentation ...
7 years, 2 months ago (2013-01-12 11:34:24 UTC) #4
divoxx
Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
7 years, 2 months ago (2013-01-12 11:35:04 UTC) #5
divoxx
Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
7 years, 2 months ago (2013-01-12 11:50:39 UTC) #6
divoxx
Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
7 years, 2 months ago (2013-01-12 23:28:31 UTC) #7
remyoudompheng
https://codereview.appspot.com/7097048/diff/9002/src/cmd/vet/atomic.go File src/cmd/vet/atomic.go (right): https://codereview.appspot.com/7097048/diff/9002/src/cmd/vet/atomic.go#newcode1 src/cmd/vet/atomic.go:1: // Copyright 2012 The Go Authors. All rights reserved. ...
7 years, 2 months ago (2013-01-12 23:48:01 UTC) #8
divoxx
On 2013/01/12 23:48:01, remyoudompheng wrote: > https://codereview.appspot.com/7097048/diff/9002/src/cmd/vet/atomic.go > File src/cmd/vet/atomic.go (right): > > https://codereview.appspot.com/7097048/diff/9002/src/cmd/vet/atomic.go#newcode1 > ...
7 years, 2 months ago (2013-01-12 23:51:20 UTC) #9
divoxx
Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
7 years, 2 months ago (2013-01-13 02:31:02 UTC) #10
divoxx
On 2013/01/13 02:31:02, divoxx wrote: > Hello mailto:dvyukov@google.com, mailto:golang-dev@googlegroups.com, mailto:remyoudompheng@gmail.com > (cc: mailto:golang-dev@googlegroups.com), > > ...
7 years, 2 months ago (2013-01-13 02:36:37 UTC) #11
remyoudompheng
No, the problem is that your code panicked on my examples because you hadn't addressed ...
7 years, 2 months ago (2013-01-13 05:42:27 UTC) #12
divoxx
Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
7 years, 2 months ago (2013-01-13 06:00:13 UTC) #13
rsc
Add this: func (f *File) gofmt(x ast.Expr) string { f.buf.Reset() printer.Fprint(&f.b, f.fset, x) return f.String() ...
7 years, 2 months ago (2013-01-18 20:36:51 UTC) #14
divoxx
Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
7 years, 2 months ago (2013-01-21 16:42:52 UTC) #15
divoxx
Russ, I changed the implementation to do as you suggested. Let me know if that ...
7 years, 2 months ago (2013-01-21 16:44:15 UTC) #16
rsc
There should be a 'Fixes issue NNN.' at the bottom of the CL description, right? ...
7 years, 2 months ago (2013-01-23 03:20:24 UTC) #17
divoxx
Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
7 years, 2 months ago (2013-01-23 07:37:07 UTC) #18
rsc
LGTM
7 years, 2 months ago (2013-01-29 23:21:12 UTC) #19
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=c8cd1f608cd2 *** cmd/vet: detect misuse of atomic.Add* Re-assigning the return value of ...
7 years, 2 months ago (2013-01-30 15:57:15 UTC) #20
divoxx
7 years, 1 month ago (2013-03-01 17:07:27 UTC) #21
*** Abandoned ***
Sign in to reply to this message.

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