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

Issue 98850043: code review 98850043: cmd/vet: check that string literals in regexp.Compile a... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 11 months ago by djd
Modified:
5 years, 9 months ago
Reviewers:
r, dsymonds, mtj1, josharian, alsodave
CC:
golang-codereviews
Visibility:
Public.

Description

cmd/vet: check that string literals in regexp.Compile are valid.

Patch Set 1 #

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

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

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

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

Total comments: 10

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

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

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

Total comments: 8

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

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

Total comments: 2

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+97 lines, -0 lines) Patch
A cmd/vet/regexp.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +59 lines, -0 lines 0 comments Download
A cmd/vet/testdata/regexp.go View 1 2 3 4 5 6 7 8 1 chunk +38 lines, -0 lines 0 comments Download

Messages

Total messages: 16
djd
Hello dsymonds@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.tools
5 years, 9 months ago (2014-06-12 05:25:02 UTC) #1
josharian
I'm not sure that this warrants a vet check, since users should be checking errors ...
5 years, 9 months ago (2014-06-12 16:27:38 UTC) #2
djd
All done, and sync'd to head. https://codereview.appspot.com/98850043/diff/80001/cmd/vet/regexp.go File cmd/vet/regexp.go (right): https://codereview.appspot.com/98850043/diff/80001/cmd/vet/regexp.go#newcode18 cmd/vet/regexp.go:18: if !vet("regexp") { ...
5 years, 9 months ago (2014-06-13 03:09:39 UTC) #3
djd
I've just updated this to account for dsymond's refactor of cmd/vet.
5 years, 9 months ago (2014-06-13 07:25:25 UTC) #4
dsymonds
LGTM, but wait for r to weigh in. https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go File cmd/vet/regexp.go (right): https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go#newcode34 cmd/vet/regexp.go:34: if ...
5 years, 9 months ago (2014-06-13 07:41:29 UTC) #5
djd
https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go File cmd/vet/regexp.go (right): https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go#newcode34 cmd/vet/regexp.go:34: if i, ok := x.X.(*ast.Ident); !ok || i.Name != ...
5 years, 9 months ago (2014-06-13 08:28:26 UTC) #6
dsymonds
LGTM https://codereview.appspot.com/98850043/diff/180001/cmd/vet/regexp.go File cmd/vet/regexp.go (right): https://codereview.appspot.com/98850043/diff/180001/cmd/vet/regexp.go#newcode57 cmd/vet/regexp.go:57: f.Badf(lit.Pos(), err.Error()) either use f.Bad, or use a ...
5 years, 9 months ago (2014-06-13 09:22:04 UTC) #7
djd
https://codereview.appspot.com/98850043/diff/180001/cmd/vet/regexp.go File cmd/vet/regexp.go (right): https://codereview.appspot.com/98850043/diff/180001/cmd/vet/regexp.go#newcode57 cmd/vet/regexp.go:57: f.Badf(lit.Pos(), err.Error()) On 2014/06/13 09:22:04, dsymonds wrote: > either ...
5 years, 9 months ago (2014-06-13 10:40:14 UTC) #8
r
I don't think this is worth it, since it's checking something that's already verifiable by ...
5 years, 9 months ago (2014-06-13 13:20:27 UTC) #9
josharian
I ran this on a large corpus. It found only one instance: github.com/coocood/jas/router_test.go:161: error parsing ...
5 years, 9 months ago (2014-06-13 15:52:45 UTC) #10
mtj1
Rob, What about a Goadvise or Golint for more general static analysis? (fork govet and ...
5 years, 9 months ago (2014-06-13 19:52:18 UTC) #11
mtj1
"One man's toy is another's real system, and every tool adds to the arsenal." On ...
5 years, 9 months ago (2014-06-13 19:53:55 UTC) #12
alsodave
I was mostly thinking of this check in the context of vet being used as ...
5 years, 9 months ago (2014-06-19 03:56:04 UTC) #13
r
If it's code review you want, suggest it for golint instead of vet. -rob On ...
5 years, 9 months ago (2014-06-19 05:27:32 UTC) #14
r
NOT LGTM Clearing this from the dashboard. Perhaps it belongs in golint? But I don' ...
5 years, 9 months ago (2014-06-26 18:17:35 UTC) #15
djd
5 years, 9 months ago (2014-06-26 23:53:14 UTC) #16
Message was sent while issue was closed.
*** Abandoned ***
Sign in to reply to this message.

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