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

Issue 4806567450902528: Issue 1794 - add handling of std::vector<std::string> by Communication::{Input,Output}Buffer (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 1 month ago by sergei
Modified:
4 years, 11 months ago
Reviewers:
Eric, Oleksandr
Visibility:
Public.

Patch Set 1 #

Total comments: 5

Patch Set 2 : change tests #

Total comments: 2

Patch Set 3 : remove WriteStrings and ReadStrings #

Total comments: 11
Unified diffs Side-by-side diffs Delta from patch set Stats (+85 lines, -29 lines) Patch
M src/engine/Main.cpp View 1 2 3 chunks +2 lines, -11 lines 0 comments Download
M src/plugin/AdblockPlusClient.cpp View 1 2 3 chunks +8 lines, -17 lines 2 comments Download
M src/shared/Communication.h View 5 chunks +29 lines, -1 line 6 comments Download
M src/shared/Utils.h View 1 2 2 chunks +2 lines, -0 lines 2 comments Download
M src/shared/Utils.cpp View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
M test/CommunicationTest.cpp View 1 1 chunk +36 lines, -0 lines 1 comment Download

Messages

Total messages: 17
sergei
5 years ago (2015-01-13 14:57:08 UTC) #1
Eric
http://codereview.adblockplus.org/4806567450902528/diff/5629499534213120/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/4806567450902528/diff/5629499534213120/src/shared/Communication.h#newcode41 src/shared/Communication.h:41: TYPE_PROC, TYPE_STRING, TYPE_WSTRING, TYPE_INT64, TYPE_INT32, TYPE_BOOL, TYPE_STRINGS We should ...
5 years ago (2015-01-13 16:20:45 UTC) #2
sergei
http://codereview.adblockplus.org/4806567450902528/diff/5629499534213120/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/4806567450902528/diff/5629499534213120/src/shared/Communication.h#newcode41 src/shared/Communication.h:41: TYPE_PROC, TYPE_STRING, TYPE_WSTRING, TYPE_INT64, TYPE_INT32, TYPE_BOOL, TYPE_STRINGS On 2015/01/13 ...
4 years, 11 months ago (2015-01-27 15:28:38 UTC) #3
Eric
On 2015/01/27 15:28:38, sergei wrote: > I'm confused, we recently accepted https://issues.adblockplus.org/ticket/715 . Just saw ...
4 years, 11 months ago (2015-02-02 15:02:31 UTC) #4
Oleksandr
I might be missing something, but why do we need both ReadStrings in AdblockPlusClient.cpp, WriteStrings ...
4 years, 11 months ago (2015-02-04 20:49:37 UTC) #5
sergei
On 2015/02/04 20:49:37, Oleksandr wrote: > I might be missing something, but why do we ...
4 years, 11 months ago (2015-02-05 12:54:50 UTC) #6
Oleksandr
> > I might be missing something, but why do we need both ReadStrings in ...
4 years, 11 months ago (2015-02-06 15:12:46 UTC) #7
sergei
On 2015/02/06 15:12:46, Oleksandr wrote: > > > I might be missing something, but why ...
4 years, 11 months ago (2015-02-06 15:20:37 UTC) #8
sergei
Sorry, now I see ReadStrings in AdblockPlusClient.cpp and WriteStrings in Main.cpp. Yes, it makes sense ...
4 years, 11 months ago (2015-02-09 11:48:55 UTC) #9
sergei
On 2015/02/09 11:48:55, sergei wrote: > Sorry, now I see ReadStrings in AdblockPlusClient.cpp and WriteStrings ...
4 years, 11 months ago (2015-02-09 14:05:37 UTC) #10
Eric
Now that the code is clearer, it has become clear that there's a design flaw ...
4 years, 11 months ago (2015-02-09 16:18:57 UTC) #11
sergei
I'm pretty sure it's beyond the scope of this issue, so could you move it ...
4 years, 11 months ago (2015-02-09 16:55:37 UTC) #12
Eric
On 2015/02/09 16:55:37, sergei wrote: > I'm pretty sure it's beyond the scope of this ...
4 years, 11 months ago (2015-02-09 17:33:52 UTC) #13
Eric
http://codereview.adblockplus.org/4806567450902528/diff/5700735861784576/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/4806567450902528/diff/5700735861784576/src/shared/Communication.h#newcode87 src/shared/Communication.h:87: buffer.read(reinterpret_cast<char*>(data.get()), sizeof(T::value_type) * length); On 2015/02/09 16:55:37, sergei wrote: ...
4 years, 11 months ago (2015-02-09 17:34:02 UTC) #14
Oleksandr
On 2015/02/09 17:33:52, Eric wrote: > On 2015/02/09 16:55:37, sergei wrote: > > I'm pretty ...
4 years, 11 months ago (2015-02-16 07:31:37 UTC) #15
Oleksandr
LGTM
4 years, 11 months ago (2015-02-17 12:23:10 UTC) #16
Eric
4 years, 11 months ago (2015-02-17 17:18:35 UTC) #17
For reference, the concerns I had are being addressed in this review:
    http://codereview.adblockplus.org/4882650246414336/

I have one reservation about implementation of a unit test, but we can address
it in the next review.

Neither of these caveats need to block this. LGTM.

http://codereview.adblockplus.org/4806567450902528/diff/5700735861784576/test...
File test/CommunicationTest.cpp (right):

http://codereview.adblockplus.org/4806567450902528/diff/5700735861784576/test...
test/CommunicationTest.cpp:120: Communication::InputBuffer
inputBuffer(outputBuffer.Get());
I don't like the use of Get() here, but I'll save comments for the subsequent
change set.
Sign in to reply to this message.

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