Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 4882650246414336: Issue 2005 - Refactor working with strings in InputBuffer and OutputBuffer

Created:
Feb. 17, 2015, 12:36 p.m. by sergei
Modified:
June 15, 2015, 7:28 a.m.
Reviewers:
Eric, Oleksandr
Visibility:
Public.

Patch Set 1 #

Total comments: 36

Patch Set 2 : address some comments #

Patch Set 3 : fix forgotten sizeof and add check for string length #

Total comments: 13

Patch Set 4 : rebase #

Patch Set 5 : address some comments #

Patch Set 6 : rebase #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+296 lines, -139 lines) Patch
M src/plugin/AdblockPlusClient.cpp View 1 2 3 4 5 16 chunks +32 lines, -38 lines 0 comments Download
M src/shared/Communication.h View 1 2 3 4 5 6 chunks +34 lines, -20 lines 0 comments Download
M test/CommunicationTest.cpp View 1 2 3 4 5 1 chunk +230 lines, -81 lines 3 comments Download

Messages

Total messages: 10
sergei
Feb. 17, 2015, 1:03 p.m. (2015-02-17 13:03:45 UTC) #1
Eric
Lots of comments, but none for AdblockPlusClient.cpp; that file looks so much better. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/shared/Communication.h File ...
Feb. 17, 2015, 6:15 p.m. (2015-02-17 18:15:12 UTC) #2
sergei
http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/shared/Communication.h#newcode98 src/shared/Communication.h:98: value.resize(length, '\0'); On 2015/02/17 18:15:13, Eric wrote: > We ...
Feb. 18, 2015, 2:13 p.m. (2015-02-18 14:13:06 UTC) #3
sergei
http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/shared/Communication.h#newcode99 src/shared/Communication.h:99: buffer.read(&value[0], sizeof(std::string::value_type) * length); In addition, Because of http://en.cppreference.com/w/cpp/string/basic_string/operator_at ...
Feb. 18, 2015, 2:41 p.m. (2015-02-18 14:41:39 UTC) #4
Eric
Looking good. There's one statement in Communication.h that should be fixed. The names of the ...
Feb. 18, 2015, 6:12 p.m. (2015-02-18 18:12:46 UTC) #5
Eric
P.S. ... Oh, and that one possibility for an infinite loop that needs a fix.
Feb. 18, 2015, 6:13 p.m. (2015-02-18 18:13:33 UTC) #6
sergei
http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/shared/Communication.h#newcode99 src/shared/Communication.h:99: buffer.read(&value[0], sizeof(std::string::value_type) * length); On 2015/02/18 18:12:47, Eric wrote: ...
Feb. 19, 2015, 5:26 p.m. (2015-02-19 17:26:56 UTC) #7
Oleksandr
LGTM
March 3, 2015, 6:45 p.m. (2015-03-03 18:45:18 UTC) #8
Eric
Looks fine except for one confusing placement of an assertion in one of the tests. ...
May 14, 2015, 1:53 p.m. (2015-05-14 13:53:38 UTC) #9
sergei
May 15, 2015, 9:45 a.m. (2015-05-15 09:45:55 UTC) #10
http://codereview.adblockplus.org/4882650246414336/diff/5743114304094208/test...
File test/CommunicationTest.cpp (right):

http://codereview.adblockplus.org/4882650246414336/diff/5743114304094208/test...
test/CommunicationTest.cpp:44: for (int i = 0; !client && i < 100; ++i)
On 2015/05/14 13:53:38, Eric wrote:
> There's no need to guard the loop with the term "!client" if you move the
> assertion below to appear before the declaration of 'clientThread'

Before declaration of `clientThread` `client` is always `nullptr`.

Powered by Google App Engine
This is Rietveld