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

Issue 6852057: code review 6852057: net: remove unused nil check (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 4 months ago by dfc
Modified:
11 years, 4 months ago
Reviewers:
CC:
mikio, bradfitz, dvyukov, rsc, golang-dev
Visibility:
Public.

Description

net: remove unused nil check This is part 1 of a series of proposals to fix issue 4369. In resolving issue 3507 it was decided not to nil out the inner conn.fd field to avoid a race. This implies the checks for fd == nil inside incref/decref are never true. Removing this logic removes one source of errClosing error values, which affects issue 4373 and moves towards bradfitz's request that fd.accept() return io.EOF when closed concurrently. Update issue 4369. Update issue 4373.

Patch Set 1 #

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

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

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

Patch Set 5 : diff -r 11094b97d92a https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -9 lines) Patch
M src/pkg/net/fd_unix.go View 1 2 chunks +0 lines, -6 lines 0 comments Download
M src/pkg/net/unixsock_posix.go View 1 1 chunk +1 line, -3 lines 0 comments Download

Messages

Total messages: 9
dfc
Hello mikioh.mikioh@gmail.com, brad@fitzpat.com, dvyukov@google.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
11 years, 4 months ago (2012-11-16 01:16:45 UTC) #1
mikio
net.Conn represents a combo of protocol and operations. conn traverses across different protocols for net.Conn. ...
11 years, 4 months ago (2012-11-16 01:58:37 UTC) #2
dvyukov
Checking 'this' for nil always looked suspicious to me. LGTM if it breaks nothing :)
11 years, 4 months ago (2012-11-16 08:19:42 UTC) #3
dfc
On 2012/11/16 08:19:42, dvyukov wrote: > Checking 'this' for nil always looked suspicious to me. ...
11 years, 4 months ago (2012-11-18 04:01:25 UTC) #4
bradfitz
LGTM I like the change but I'm suspicious why it was zeroed in the first ...
11 years, 4 months ago (2012-11-18 04:05:53 UTC) #5
dfc
It used to be zeroed as a form of 'is closed' check, which we removed ...
11 years, 4 months ago (2012-11-18 04:09:03 UTC) #6
bradfitz
Cool. On Sat, Nov 17, 2012 at 8:09 PM, Dave Cheney <dave@cheney.net> wrote: > It ...
11 years, 4 months ago (2012-11-18 04:11:56 UTC) #7
dfc
*** Submitted as http://code.google.com/p/go/source/detail?r=bbc0024af4a4 *** net: remove unused nil check This is part 1 of ...
11 years, 4 months ago (2012-11-18 04:33:27 UTC) #8
rsc
11 years, 4 months ago (2012-11-19 13:24:58 UTC) #9
On Fri, Nov 16, 2012 at 3:19 AM,  <dvyukov@google.com> wrote:
> Checking 'this' for nil always looked suspicious to me.

Erase that from your mind. Checking a pointer-valued receiver for nil
is perfectly fine in Go, just like checking an integer-valued receiver
for 0 is fine in Go.

In brain-damaged languages like C++ on the other hand...

Russ
Sign in to reply to this message.

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