go.tools/cmd/vet: validate calls to t.Log and t.Logf
Be careful not to complain about math.Log and cmplx.Log.
Seems worthwhile since t.Log and t.Logf are often written but
rarely executed.
Nothing new turned up in the standard library.
Fixes issue 8504.
Code looks good. Here are the results on my standard corpus. True positives: camlistore.org/third_party/github.com/camlistore/goexif/tiff/tiff_test.go:215: wrong ...
9 years, 7 months ago
(2014-08-22 22:42:15 UTC)
#2
Code looks good. Here are the results on my standard corpus.
True positives:
camlistore.org/third_party/github.com/camlistore/goexif/tiff/tiff_test.go:215:
wrong number of args for format in Logf call: 1 needed but 2 args
bitbucket.org/cdk/sets/sets_test.go:193: possible formatting directive in Log
call
Questionable -- local definition of logf that plays by different rules:
code.google.com/p/appengine-go/appengine/aetest/context.go:160: no formatting
directive in logf call
code.google.com/p/appengine-go/appengine/aetest/context.go:161: no formatting
directive in logf call
code.google.com/p/appengine-go/appengine/aetest/context.go:162: no formatting
directive in logf call
code.google.com/p/appengine-go/appengine/aetest/context.go:163: no formatting
directive in logf call
code.google.com/p/appengine-go/appengine/aetest/context.go:164: no formatting
directive in logf call
code.google.com/p/appengine-go/appengine/remote_api/client.go:69: no formatting
directive in logf call
code.google.com/p/appengine-go/appengine/remote_api/client.go:70: no formatting
directive in logf call
code.google.com/p/appengine-go/appengine/remote_api/client.go:71: no formatting
directive in logf call
code.google.com/p/appengine-go/appengine/remote_api/client.go:72: no formatting
directive in logf call
code.google.com/p/appengine-go/appengine/remote_api/client.go:73: no formatting
directive in logf call
code.google.com/p/appengine-go/appengine_internal/api_dev.go:316: constant 0 not
a string in call to logf
code.google.com/p/appengine-go/appengine_internal/api_dev.go:317: constant 1 not
a string in call to logf
code.google.com/p/appengine-go/appengine_internal/api_dev.go:318: constant 2 not
a string in call to logf
code.google.com/p/appengine-go/appengine_internal/api_dev.go:319: constant 3 not
a string in call to logf
code.google.com/p/appengine-go/appengine_internal/api_dev.go:321: constant 4 not
a string in call to logf
bitbucket.org/teythoon/vgo/src/vgo/log/interface.go:72: no formatting directive
in logf call
bitbucket.org/teythoon/vgo/src/vgo/log/interface.go:79: no formatting directive
in logf call
bitbucket.org/teythoon/vgo/src/vgo/log/interface.go:86: no formatting directive
in logf call
bitbucket.org/teythoon/vgo/src/vgo/log/interface.go:93: no formatting directive
in logf call
bitbucket.org/teythoon/vgo/src/vgo/log/interface.go:100: no formatting directive
in logf call
bitbucket.org/teythoon/vgo/src/vgo/log/interface.go:107: no formatting directive
in logf call
Thoughts?
You could use -printfuncs to tell it about your logf (-printfuncs=logf:2), which should override the ...
9 years, 7 months ago
(2014-08-22 23:27:14 UTC)
#3
You could use -printfuncs to tell it about your logf
(-printfuncs=logf:2), which should override the installed definition.
-rob
On Fri, Aug 22, 2014 at 3:42 PM, <josharian@gmail.com> wrote:
> Code looks good. Here are the results on my standard corpus.
>
> True positives:
>
>
camlistore.org/third_party/github.com/camlistore/goexif/tiff/tiff_test.go:215:
> wrong number of args for format in Logf call: 1 needed but 2 args
> bitbucket.org/cdk/sets/sets_test.go:193: possible formatting directive
> in Log call
>
> Questionable -- local definition of logf that plays by different rules:
>
> code.google.com/p/appengine-go/appengine/aetest/context.go:160: no
> formatting directive in logf call
> code.google.com/p/appengine-go/appengine/aetest/context.go:161: no
> formatting directive in logf call
> code.google.com/p/appengine-go/appengine/aetest/context.go:162: no
> formatting directive in logf call
> code.google.com/p/appengine-go/appengine/aetest/context.go:163: no
> formatting directive in logf call
> code.google.com/p/appengine-go/appengine/aetest/context.go:164: no
> formatting directive in logf call
> code.google.com/p/appengine-go/appengine/remote_api/client.go:69: no
> formatting directive in logf call
> code.google.com/p/appengine-go/appengine/remote_api/client.go:70: no
> formatting directive in logf call
> code.google.com/p/appengine-go/appengine/remote_api/client.go:71: no
> formatting directive in logf call
> code.google.com/p/appengine-go/appengine/remote_api/client.go:72: no
> formatting directive in logf call
> code.google.com/p/appengine-go/appengine/remote_api/client.go:73: no
> formatting directive in logf call
> code.google.com/p/appengine-go/appengine_internal/api_dev.go:316:
> constant 0 not a string in call to logf
> code.google.com/p/appengine-go/appengine_internal/api_dev.go:317:
> constant 1 not a string in call to logf
> code.google.com/p/appengine-go/appengine_internal/api_dev.go:318:
> constant 2 not a string in call to logf
> code.google.com/p/appengine-go/appengine_internal/api_dev.go:319:
> constant 3 not a string in call to logf
> code.google.com/p/appengine-go/appengine_internal/api_dev.go:321:
> constant 4 not a string in call to logf
> bitbucket.org/teythoon/vgo/src/vgo/log/interface.go:72: no formatting
> directive in logf call
> bitbucket.org/teythoon/vgo/src/vgo/log/interface.go:79: no formatting
> directive in logf call
> bitbucket.org/teythoon/vgo/src/vgo/log/interface.go:86: no formatting
> directive in logf call
> bitbucket.org/teythoon/vgo/src/vgo/log/interface.go:93: no formatting
> directive in logf call
> bitbucket.org/teythoon/vgo/src/vgo/log/interface.go:100: no formatting
> directive in logf call
> bitbucket.org/teythoon/vgo/src/vgo/log/interface.go:107: no formatting
> directive in logf call
>
> Thoughts?
>
>
> https://codereview.appspot.com/130490043/
r=dsymonds LGTM but wait for dsymonds, please > You could use -printfuncs to tell it ...
9 years, 7 months ago
(2014-08-23 00:00:46 UTC)
#4
r=dsymonds
LGTM but wait for dsymonds, please
> You could use -printfuncs to tell it about your logf
> (-printfuncs=logf:2), which should override the installed definition.
SGTM but as bunch of these are in appengine code, I'd like to defer to dsymonds.
LGTM This is fine. There's plenty of other places that do something like x.Logf(<something>, "%d", ...
9 years, 7 months ago
(2014-08-23 00:06:13 UTC)
#5
LGTM
This is fine. There's plenty of other places that do something like
x.Logf(<something>, "%d", ...) where the <something> is a logging level or
similar. That could be solved in a different way (i.e. peeking if the second arg
looks more like a format string), but that'll wait for another time.
*** Submitted as https://code.google.com/p/go/source/detail?r=32d1e9227ea0&repo=tools *** go.tools/cmd/vet: validate calls to t.Log and t.Logf Be careful not ...
9 years, 7 months ago
(2014-08-25 19:31:48 UTC)
#6
*** Submitted as
https://code.google.com/p/go/source/detail?r=32d1e9227ea0&repo=tools ***
go.tools/cmd/vet: validate calls to t.Log and t.Logf
Be careful not to complain about math.Log and cmplx.Log.
Seems worthwhile since t.Log and t.Logf are often written but
rarely executed.
Nothing new turned up in the standard library.
Fixes issue 8504.
LGTM=josharian, dsymonds
R=golang-codereviews, josharian, dsymonds
CC=golang-codereviews
https://codereview.appspot.com/130490043
Issue 130490043: code review 130490043: go.tools/cmd/vet: validate calls to t.Log and t.Logf
(Closed)
Created 9 years, 7 months ago by r
Modified 9 years, 7 months ago
Reviewers:
Base URL:
Comments: 0