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

Issue 53470043: code review 53470043: syscall: add Flock_t.Lock method (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 2 months ago by bradfitz
Modified:
10 years, 2 months ago
Reviewers:
rsc, iant
CC:
golang-codereviews, iant, mikio
Visibility:
Public.

Description

syscall: add Flock_t.Lock method Fixes Issue 7059

Patch Set 1 #

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

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -2 lines) Patch
M src/pkg/syscall/consistency_unix_test.go View 1 1 chunk +2 lines, -2 lines 0 comments Download
A src/pkg/syscall/flock.go View 1 2 1 chunk +22 lines, -0 lines 0 comments Download
A src/pkg/syscall/flock_linux_32bit.go View 1 2 1 chunk +13 lines, -0 lines 0 comments Download

Messages

Total messages: 6
bradfitz
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
10 years, 2 months ago (2014-01-17 01:45:20 UTC) #1
iant
LGTM
10 years, 2 months ago (2014-01-17 15:35:06 UTC) #2
bradfitz
I will submit this Tuesday or Wednesday if nobody voices an opinion about a method ...
10 years, 2 months ago (2014-01-17 17:21:38 UTC) #3
mikio
Is there any reason not to provide a file locking API in the package os? ...
10 years, 2 months ago (2014-01-18 08:42:08 UTC) #4
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=769c936aa544 *** syscall: add Flock_t.Lock method Fixes Issue 7059 R=golang-codereviews, iant, mikioh.mikioh ...
10 years, 2 months ago (2014-01-21 22:52:46 UTC) #5
rsc
10 years, 2 months ago (2014-01-21 23:50:59 UTC) #6
Message was sent while issue was closed.
Why is this a method? There are no method-wrapped system calls in this package,
except for some Windows things that need some wrapped state. In particular, we
didn't use methods for SetsockoptLinger etc; what makes Flock_t so special to
deserve a method?

Even if package syscall did use methods, it seems a very unconventional use.
Usually the receiver is the thing being operated on. The thing being operated on
here is the file descriptor; the Flock is just one piece of data being passed to
the call. lk.Lock(fd, XXX) reminds me of Python's ','.join(list).

Please make this func FcntlFlock instead, to match what has already been done
for Setsockopt, and to avoid forcing the use of methods (and method tables) in
package syscall. It's not a big deal in this one single-method type, but I do
not want to establish a precedent.

I notice that in the review you did ask if anyone had any comments about methods
vs functions, but that only went to Ian. Remember that golang-codereviews is a
write-only mailing list and not one that anyone is guaranteed to read. If you
want to solicit feedback from more people, please add them to the review or post
a separate mail to golang-dev.

Thanks.
Sign in to reply to this message.

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