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

Issue 4357052: code review 4357052: os: New Open API. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 11 months ago by r
Modified:
5 years, 11 months ago
Reviewers:
rog
CC:
golang-dev, bradfitzgoog, rsc, r2
Visibility:
Public.

Description

os: New Open API. We replace the current Open with: OpenFile(name, flag, perm) // same as old Open Open(name) // same as old Open(name, O_RDONLY, 0) Create(name) // same as old Open(name, O_RDWR|O_TRUNC|O_CREAT, 0666) This CL includes a gofix module and full code updates: all.bash passes. (There may be a few comments I missed.) The interesting packages are: gofix os Everything else is automatically generated except for hand tweaks to: src/pkg/io/ioutil/ioutil.go src/pkg/io/ioutil/tempfile.go src/pkg/crypto/tls/generate_cert.go src/cmd/goyacc/goyacc.go src/cmd/goyacc/units.y

Patch Set 1 #

Total comments: 7

Patch Set 2 : diff -r dc1d5042801a https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r dc1d5042801a https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r dc1d5042801a https://go.googlecode.com/hg/ #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+291 lines, -106 lines) Patch
M misc/dashboard/builder/exec.go View 1 1 chunk +1 line, -1 line 1 comment Download
M misc/goplay/goplay.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/cgo/main.go View 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/cgo/util.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/godoc/codewalk.go View 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/godoc/index.go View 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/godoc/utils.go View 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/gofix/Makefile View 1 chunk +1 line, -0 lines 0 comments Download
M src/cmd/gofix/main.go View 1 chunk +1 line, -1 line 0 comments Download
A src/cmd/gofix/osopen.go View 1 1 chunk +109 lines, -0 lines 0 comments Download
A src/cmd/gofix/osopen_test.go View 1 1 chunk +53 lines, -0 lines 0 comments Download
M src/cmd/gofmt/gofmt.go View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/cmd/goinstall/main.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/goinstall/parse.go View 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/gotest/gotest.go View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/cmd/gotype/gotype.go View 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/goyacc/goyacc.go View 1 4 chunks +7 lines, -7 lines 0 comments Download
M src/cmd/goyacc/units.y View 1 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/archive/tar/reader_test.go View 4 chunks +4 lines, -4 lines 0 comments Download
M src/pkg/archive/zip/reader.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/compress/lzw/writer_test.go View 2 chunks +2 lines, -2 lines 0 comments Download
M src/pkg/compress/zlib/writer_test.go View 2 chunks +2 lines, -2 lines 0 comments Download
M src/pkg/crypto/rand/rand_unix.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/crypto/tls/generate_cert.go View 1 2 chunks +2 lines, -2 lines 1 comment Download
M src/pkg/debug/elf/file.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/debug/macho/file.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/debug/pe/file.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/debug/proc/proc_linux.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/exec/exec.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/exp/draw/x11/auth.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/exp/ogle/cmd.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/go/parser/interface.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/gob/dump.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/html/parse_test.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/http/fs.go View 2 chunks +2 lines, -2 lines 0 comments Download
M src/pkg/image/decode_test.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/image/png/reader_test.go View 2 chunks +2 lines, -2 lines 0 comments Download
M src/pkg/io/ioutil/ioutil.go View 1 3 chunks +3 lines, -3 lines 0 comments Download
M src/pkg/io/ioutil/tempfile.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/mime/type.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/net/parse.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/net/parse_test.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/os/env_plan9.go View 1 3 chunks +3 lines, -3 lines 0 comments Download
M src/pkg/os/file.go View 1 2 2 chunks +19 lines, -3 lines 0 comments Download
M src/pkg/os/file_plan9.go View 1 1 chunk +7 lines, -5 lines 0 comments Download
M src/pkg/os/file_unix.go View 1 chunk +5 lines, -3 lines 0 comments Download
M src/pkg/os/file_windows.go View 1 2 chunks +6 lines, -4 lines 0 comments Download
M src/pkg/os/getwd.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/os/inotify/inotify_linux_test.go View 1 1 chunk +1 line, -1 line 1 comment Download
M src/pkg/os/os_test.go View 1 15 chunks +15 lines, -15 lines 0 comments Download
M src/pkg/os/path.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/os/path_test.go View 1 4 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/os/sys_linux.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/os/sys_plan9.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/path/filepath/match.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/path/filepath/path.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/path/filepath/path_test.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/strconv/fp_test.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/testing/testing.go View 1 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 17
r
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
5 years, 11 months ago (2011-04-04 22:14:13 UTC) #1
bradfitzgoog
http://codereview.appspot.com/4357052/diff/1/src/pkg/os/file_windows.go File src/pkg/os/file_windows.go (right): http://codereview.appspot.com/4357052/diff/1/src/pkg/os/file_windows.go#newcode150 src/pkg/os/file_windows.go:150: if e == syscall.ERROR_NOMORE_FILES { I think this was ...
5 years, 11 months ago (2011-04-04 22:17:05 UTC) #2
rsc
I stopped reading after misc/goplay/goplay.go. Please add that as a test case. http://codereview.appspot.com/4357052/diff/1/misc/goplay/goplay.go File misc/goplay/goplay.go ...
5 years, 11 months ago (2011-04-04 22:45:44 UTC) #3
rsc
Also, it may be worth warning (call warn) about calls to Open with O_CREAT that ...
5 years, 11 months ago (2011-04-04 22:52:21 UTC) #4
r2
On Apr 4, 2011, at 3:45 PM, rsc@golang.org wrote: > I stopped reading after misc/goplay/goplay.go. ...
5 years, 11 months ago (2011-04-04 22:54:14 UTC) #5
rsc
> Really? That seems unsafe - shared state and all that. Where does the ast ...
5 years, 11 months ago (2011-04-04 22:56:11 UTC) #6
r
http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go File src/cmd/gofix/osopen.go (right): http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go#newcode84 src/cmd/gofix/osopen.go:84: return isPkgDot(flag, "os", "O_CREAT") On 2011/04/04 22:45:44, rsc wrote: ...
5 years, 11 months ago (2011-04-04 22:56:17 UTC) #7
r2
On Apr 4, 2011, at 3:56 PM, Russ Cox wrote: >> Really? That seems unsafe ...
5 years, 11 months ago (2011-04-04 22:57:13 UTC) #8
r
Hello golang-dev@googlegroups.com, bradfitzwork, rsc, r2 (cc: golang-dev@googlegroups.com), Please take another look.
5 years, 11 months ago (2011-04-05 04:53:36 UTC) #9
r2
Updated and all comments addressed. I added a copule of warnings about dangerous conditions to ...
5 years, 11 months ago (2011-04-05 04:54:54 UTC) #10
r2
New addendum to CL desc: Everything else is automatically generated except for hand tweaks to: ...
5 years, 11 months ago (2011-04-05 04:56:32 UTC) #11
rsc
LGTM Well that's a win.
5 years, 11 months ago (2011-04-05 05:15:04 UTC) #12
r
*** Submitted as http://code.google.com/p/go/source/detail?r=6c96d1022285 *** os: New Open API. We replace the current Open with: ...
5 years, 11 months ago (2011-04-05 06:42:19 UTC) #13
rog
http://codereview.appspot.com/4357052/diff/11001/misc/dashboard/builder/exec.go File misc/dashboard/builder/exec.go (right): http://codereview.appspot.com/4357052/diff/11001/misc/dashboard/builder/exec.go#newcode52 misc/dashboard/builder/exec.go:52: f, err := os.Create(logfile) Did you really intend to ...
5 years, 11 months ago (2011-04-05 08:40:58 UTC) #14
rsc
> http://codereview.appspot.com/4357052/diff/11001/misc/dashboard/builder/exec.go#newcode52 > misc/dashboard/builder/exec.go:52: f, err := os.Create(logfile) > Did you really intend to lose ...
5 years, 11 months ago (2011-04-05 14:52:41 UTC) #15
rog
On 5 April 2011 15:52, Russ Cox <rsc@golang.org> wrote: >> (BTW it looks like a ...
5 years, 11 months ago (2011-04-05 15:08:23 UTC) #16
rsc
5 years, 11 months ago (2011-04-05 15:08:48 UTC) #17
okay; send a CL


On Tue, Apr 5, 2011 at 11:08, roger peppe <rogpeppe@gmail.com> wrote:
> On 5 April 2011 15:52, Russ Cox <rsc@golang.org> wrote:
>>> (BTW it looks like a bug that the file isn't removed afterwards)
>>
>> The test file is in _obj.  It will get deleted during make clean.
>
> but if you run gotest twice in that directory i think the second
> time would fail. as far as i can tell, gotest doesn't remove _obj.
>
> if that's the case, i still think that the file should be explicitly deleted.
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 80a51fa-tainted