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

Issue 14656044: code review 14656044: database/sql: Remove redundant condition in if (Closed)

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

Description

database/sql: Remove redundant condition in if The final condition (db.maxIdleConnsLocked() > db.freeConn.Len()) can only be true iff db.maxIdleConnsLocked() is greater than 0, so previously checking if it's greater than 0 is a waste, specially when that involves a method call which (ATM) can't be inlined and includes a switch. Dissasembly follows (test for err == nil has been omitted for clarity): Before: 43c357: cmp $0x0,%bl 43c35a: jne 43c3ce <database/sql.(*DB).putConnDBLocked+0x1ce> 43c35c: mov %rax,(%rsp) 43c360: callq 43aec0 <database/sql.(*DB).maxIdleConnsLocked> 43c365: mov 0x8(%rsp),%rbx 43c36a: cmp $0x0,%rbx 43c36e: jle 43c3ce <database/sql.(*DB).putConnDBLocked+0x1ce> 43c370: mov 0x30(%rsp),%rbx 43c375: mov %rbx,(%rsp) 43c379: callq 43aec0 <database/sql.(*DB).maxIdleConnsLocked> 43c37e: mov 0x30(%rsp),%rdx 43c383: mov 0x8(%rsp),%rcx 43c388: mov 0x28(%rdx),%rbp 43c38c: mov 0x28(%rbp),%rbx 43c390: cmp %rcx,%rbx 43c393: jge 43c3ce <database/sql.(*DB).putConnDBLocked+0x1ce> 43c395: mov 0x28(%rdx),%rbp 43c399: mov %rbp,(%rsp) 43c39d: mov 0x38(%rsp),%rcx 43c3a2: mov $0x556c60,%eax 43c3a7: mov %rax,0x8(%rsp) 43c3ac: mov %rcx,0x10(%rsp) 43c3b1: callq 4db5b0 <container/list.(*List).PushFront> After: 43c357: cmp $0x0,%bl 43c35a: jne 43c3b5 <database/sql.(*DB).putConnDBLocked+0x1b5> 43c35c: mov %rax,(%rsp) 43c360: callq 43aec0 <database/sql.(*DB).maxIdleConnsLocked> 43c365: mov 0x30(%rsp),%rdx 43c36a: mov 0x8(%rsp),%rcx 43c36f: mov 0x28(%rdx),%rbp 43c373: mov 0x28(%rbp),%rbx 43c377: cmp %rcx,%rbx 43c37a: jge 43c3b5 <database/sql.(*DB).putConnDBLocked+0x1b5> 43c37c: mov 0x28(%rdx),%rbp 43c380: mov %rbp,(%rsp) 43c384: mov 0x38(%rsp),%rcx 43c389: mov $0x556c60,%eax 43c38e: mov %rax,0x8(%rsp) 43c393: mov %rcx,0x10(%rsp) 43c398: callq 4db590 <container/list.(*List).PushFront>

Patch Set 1 #

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -1 line) Patch
M src/pkg/database/sql/sql.go View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 11
Hierro
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
10 years, 5 months ago (2013-10-17 14:13:44 UTC) #1
bradfitz
This is a trivial fix, but it's also mostly harmless to keep it as-is for ...
10 years, 5 months ago (2013-10-17 21:57:38 UTC) #2
Hierro
On 2013/10/17 21:57:38, bradfitz wrote: > This is a trivial fix, but it's also mostly ...
10 years, 5 months ago (2013-10-18 10:00:36 UTC) #3
Hierro
Could we get this merged now that 1.2 is out? We have few more improvements ...
10 years, 3 months ago (2013-12-02 22:15:57 UTC) #4
bradfitz
The tree isn't open yet. On Mon, Dec 2, 2013 at 2:15 PM, <alberto.garcia.hierro@gmail.com> wrote: ...
10 years, 3 months ago (2013-12-03 18:22:12 UTC) #5
Hierro
On 2013/12/03 18:22:12, bradfitz wrote: > The tree isn't open yet. > I wasn't aware ...
10 years, 3 months ago (2013-12-05 01:24:24 UTC) #6
iant
On Wed, Dec 4, 2013 at 5:24 PM, <alberto.garcia.hierro@gmail.com> wrote: > On 2013/12/03 18:22:12, bradfitz ...
10 years, 3 months ago (2013-12-05 01:31:22 UTC) #7
Hierro
On 2013/12/05 01:31:22, iant wrote: > On Wed, Dec 4, 2013 at 5:24 PM, <mailto:alberto.garcia.hierro@gmail.com> ...
10 years, 3 months ago (2013-12-05 11:34:00 UTC) #8
bradfitz
LGTM
10 years, 3 months ago (2013-12-10 11:57:33 UTC) #9
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=5b9a2c239b6d *** database/sql: Remove redundant condition in if The final condition (db.maxIdleConnsLocked() ...
10 years, 3 months ago (2013-12-10 12:10:02 UTC) #10
Hierro
10 years, 2 months ago (2014-01-05 16:36:07 UTC) #11
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