Issue 4882650246414336: Issue 2005 - Refactor working with strings in InputBuffer and OutputBuffer
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
MessagesTotal messages: 10
Lots of comments, but none for AdblockPlusClient.cpp; that file looks so much better. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... File src/shared/Communication.h (right): http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:72: std::vector<std::string> tmpStrings; Ugh. A temporary vector is just ugly and inefficient. Nevertheless, it has correct behavior and is expedient. We need clean interfaces and automatic conversion in all the operators first. This operator can be improved later. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:98: value.resize(length, '\0'); We don't need a fill character, since we're immediately going to overwrite it. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:99: buffer.read(&value[0], sizeof(std::string::value_type) * length); We need two things here. -- An explicit const_cast to value.data(). -- A comment that we're writing into a buffer declared read-only in the standard. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:99: buffer.read(&value[0], sizeof(std::string::value_type) * length); Separately, we don't need the sizeof expression here. It's not wrong, and the optimizer will probably make it disappear, but this is std::string, and we don't need to worry about that sizeof not being 1. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:101: throw new std::runtime_error("Unexpected end of input buffer"); It wasn't your error, but we should be calling bad() here. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:172: buffer.write(value.c_str(), sizeof(std::string::value_type) * length); See sizeof comment above. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... File test/CommunicationTest.cpp (right): http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:28: TEST(CommunicationTest, PipeReadWrite) As long as we're improving the unit tests, can we separate the pipe tests from the buffer tests? As far as I'm concerned, they can stay in the same file; I also won't object if you want two files. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:42: auto serverThread = std::thread([&server] Is it necessary to use a separate thread to initialize the server? You are, after all, using the server object in the current thread below. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:46: std::this_thread::sleep_for(std::chrono::milliseconds(100)); I myself have had trouble with the Communication unit tests failing intermittently, so I appreciate the improvement. I believe there is, however, a better was of doing it than a fixed delay. The system call used for MODE_CONNECT, CreateFile(), will block if the pipe exists but isn't ready (at least as I understand it). Instead of hoping that it's ready within 100 ms, initialize the client in a separate thread and wait for it to complete. Its initialization thread will terminate as soon as the client is ready to use. This will be particularly useful to do if you split up the unit tests as I've suggested below, since you won't be multiplying the delay. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:63: Each of these individual tests on disparate variable types deserves a separate unit test. Should one ever fail, you'd want to know if only one fails or if they're all failing. The present test structure doesn't allow that distinction. Separately, it's a useful unit test to send multiple variables in sequence. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:123: void SendReceiveTypes(TestedType value, AnotherType anotherTypeValue) You're combining two kinds of test here. 1) Ensure that a value sent matches the value received. 2) Ensure that (a) a value sent is not interpretable as another type, (b) when it attempted in error, it doesn't advance the internal stream pointer. I'd separate out the first kind of test from the second, as always to partition types of failure between different tests. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:127: Communication::InputBuffer response(message.Get()); I don't particularly care for Get() in the current code base. Whatever changes might be needed to improve it aren't relevant to the present review, nor is there a need to discuss it here (although, in very brief, there's lots of extraneous copying). Get() shouldn't really be public, since it's only used to communicate with the pipe; but it is, and there's no reason to change it here. On the other hand, we can isolate Get() so that it still feels like an internal function, in the eventuality that the buffer and pipe classes are ever reworked. What's being implemented here is the moral equivalent of an "in-memory pipe", and furthermore, it's only good for a single use (which is all the code ever uses it for). So how about a utility function called 'ConvertOutputToInput' or 'SimulatePipe' or something similar? We have both copy constructor and copy assignment already defined, so the function is a one liner. And I won't quibble about efficiency for a few unit tests. I'd also like to see this for documentary reasons. It took rather too long for me to understand what Get() was doing in the first place. A good name for the utility function should be adequate documentation. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:148: TEST(CommunicationTest, SendWString_ReceiveWString) Probably better to spell out "Wide" here and below. Might also want to be explicit about "Narrow" below. Although I personally think that the underscore here is fine, it's not in our code style. How about "TypeStringSendWideReceiveWide"? Wordy but consistent and descriptive. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:207: void SendReceiveStrings(const std::vector<std::string>& src) We should test narrow-wide conversions here as well.
http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... File src/shared/Communication.h (right): http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:98: value.resize(length, '\0'); On 2015/02/17 18:15:13, Eric wrote: > We don't need a fill character, since we're immediately going to overwrite it. I've removed it but it's still used because void resize(size_type _Newsize) { resize(_Newsize, _Elem()); } http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:99: buffer.read(&value[0], sizeof(std::string::value_type) * length); On 2015/02/17 18:15:13, Eric wrote: > Separately, we don't need the sizeof expression here. It's not wrong, and the > optimizer will probably make it disappear, but this is std::string, and we don't > need to worry about that sizeof not being 1. done http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:99: buffer.read(&value[0], sizeof(std::string::value_type) * length); On 2015/02/17 18:15:13, Eric wrote: > We need two things here. > -- An explicit const_cast to value.data(). > -- A comment that we're writing into a buffer declared read-only in the > standard. I have just thought about it again and I'm not sure that we need it. We discussed it already but I cannot find it, could you remind the points again or point to the discussion, please? As far as I remember, the arguments were: 1. We cannot be sure that internally the string is stored in the contiguous buffer. 2. We could use another writable buffer (which is inefficient) because `std::string::data` returns the pointer to the "read-only" buffer and we are not supposed to be allowed to write there. 1> It's guaranteed by C++11 standard, otherwise we cannot use any feature of C++11. 2> Actually, it's a good point to use `&str[0]` instead of `const_cast`. Despite currently de facto we can use `const_cast`, there might be some reasons to avoid it. So, why do we need to write suspicious code which violates the const-correctness? http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:101: throw new std::runtime_error("Unexpected end of input buffer"); On 2015/02/17 18:15:13, Eric wrote: > It wasn't your error, but we should be calling bad() here. Right now I'm not sure about it and it's not relevant to the current changes , so it would be better to do in another commit. Could you create the issue explaining the reasons it's wrong. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:172: buffer.write(value.c_str(), sizeof(std::string::value_type) * length); On 2015/02/17 18:15:13, Eric wrote: > See sizeof comment above. done. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... File test/CommunicationTest.cpp (right): http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:28: TEST(CommunicationTest, PipeReadWrite) On 2015/02/17 18:15:13, Eric wrote: > As long as we're improving the unit tests, can we separate the pipe tests from > the buffer tests? As far as I'm concerned, they can stay in the same file; I > also won't object if you want two files. Actually I would not like to improve tests in that changeset, it's more like "I had to", so let's keep them here and do it later. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:63: On 2015/02/17 18:15:13, Eric wrote: > Each of these individual tests on disparate variable types deserves a separate > unit test. Should one ever fail, you'd want to know if only one fails or if > they're all failing. The tests of each type are below, it's not a testing of buffers, it's a testing of Pipe. Instead of testing on some completely abstract data I had to use our buffers because the Pipe functions work only with our buffers. > The present test structure doesn't allow that distinction. Actually if something fails we will get the information about it (line number, expected value, actual value). But I agree that it's quite comprehensive test rather than "unit test". > Separately, it's a useful unit test to send multiple variables in sequence. Although I find it already better than it was, should I extract it into a separate test? Actually, I would like to have it as it's now because it covers what we actually do while the client is communicating with the server. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:123: void SendReceiveTypes(TestedType value, AnotherType anotherTypeValue) On 2015/02/17 18:15:13, Eric wrote: > You're combining two kinds of test here. > 1) Ensure that a value sent matches the value received. > 2) Ensure that (a) a value sent is not interpretable as another type, (b) when > it attempted in error, it doesn't advance the internal stream pointer. > > I'd separate out the first kind of test from the second, as always to partition > types of failure between different tests. Before the everything was in one test, would you mind if we make such improvements in another commit? http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:127: Communication::InputBuffer response(message.Get()); I find it as overkill here. On 2015/02/17 18:15:13, Eric wrote: > I don't particularly care for Get() in the current code base. Whatever changes > might be needed to improve it aren't relevant to the present review, nor is > there a need to discuss it here (although, in very brief, there's lots of > extraneous copying). Get() shouldn't really be public, since it's only used to > communicate with the pipe; but it is, and there's no reason to change it here. I also feel it but actually in the current version public Get looks good. Despite the buffers are used only with Pipe, they are not named as PipeInputBuffer and PipeOutputBuffer, currently they are like some helpers to serialize our data into array of bytes. BTW, regarding it, actually Get should return std::vector, not std::string. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:148: TEST(CommunicationTest, SendWString_ReceiveWString) On 2015/02/17 18:15:13, Eric wrote: > Probably better to spell out "Wide" here and below. Might also want to be > explicit about "Narrow" below. > > Although I personally think that the underscore here is fine, it's not in our > code style. > > How about "TypeStringSendWideReceiveWide"? Wordy but consistent and descriptive. done. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:207: void SendReceiveStrings(const std::vector<std::string>& src) On 2015/02/17 18:15:13, Eric wrote: > We should test narrow-wide conversions here as well. added
http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... File src/shared/Communication.h (right): http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... 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 1) If pos == size(), the behavior is undefined. I've added `if (length > 0)` because otherwise it's UB. I've also added the test, just in case, but, I guess, it won't ever fail. From http://en.cppreference.com/w/cpp/string/basic_string/data "Modifying the character array accessed through data is undefined behavior." http://en.cppreference.com/w/cpp/string/basic_string/c_str "Writing to the character array accessed through c_str() is undefined behavior."
Looking good. There's one statement in Communication.h that should be fixed. The names of the unit tests could be more consistent. Otherwise, I'm good with patch set 3. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... File src/shared/Communication.h (right): http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:98: value.resize(length, '\0'); On 2015/02/18 14:13:06, sergei wrote: > I've removed it but it's still used That's fine. Nothing about external string initialization in the C++ library is efficient, but getting rid of the argument reads better. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:99: buffer.read(&value[0], sizeof(std::string::value_type) * length); On 2015/02/18 14:13:06, sergei wrote: > So, why do we need to write suspicious code which violates the > const-correctness? From my point of view, it is _already_ suspicious code. Using const-cast is a way of documenting the suspicion. We're relying on the particular implementation of std::string in MSVC. In other words, we're using something that only looks like standard. My main concern over this whole issue is (and has been) that we should not do this silently, that is, without documentation. It should not be a conventional idiom that passes by without comment. I'm beginning to think that the right way to treat this kind of string initialization is with our own template function. That way we can put all the documentation in one place and let the use of the function serve as the point-of-use documentation. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:99: buffer.read(&value[0], sizeof(std::string::value_type) * length); On 2015/02/18 14:41:39, sergei wrote: > From > http://en.cppreference.com/w/cpp/string/basic_string/data From that same page: "data()[i] == operator[](i) for every i in [0, size()). (until C++11)" Given this statement, there is no distinction between &str[0] and str.data(). I find std.data() a more direct expression, without the legacy burden of C idiom. You've pointed out that modifying through data() is undefined, which means that modifying through &str[0] is also undefined; no distinction on that point. There's a proviso that this is only guaranteed before C++11, but Microsoft doesn't (much) segregate C++11 and C++03 semantics, so we can expect that the proviso won't matter for us. Once we all acknowledge that we are willing to write non-standard code for the sake of expediency (and even efficiency), it will become clearer what to do about it. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:101: throw new std::runtime_error("Unexpected end of input buffer"); A separate issue for this is overkill, but very briefly, if an operation sets the bad bit it will also set the fail bit, but not vice-versa. See http://www.cplusplus.com/reference/ios/basic_ios/bad/ http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... File test/CommunicationTest.cpp (right): http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:28: TEST(CommunicationTest, PipeReadWrite) On 2015/02/18 14:13:06, sergei wrote: > Actually I would not like to improve tests in that changeset, it's more like "I > had to", so let's keep them here and do it later. OK. I am most concerned with eliminating the explicit conversions. I won't block that by insisting on perfect unit tests. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:63: On 2015/02/18 14:13:06, sergei wrote: > The tests of each type are below, it's not a testing of buffers, it's a testing > of Pipe. Instead of testing on some completely abstract data I had to use our > buffers because the Pipe functions work only with our buffers. I don't want to belabor the point here, but they're different enough to be worth testing separately. The buffer classes prepare data to move across the pipe, and the pipe class actually moves it. These two can be tested separately. > But I agree that it's quite comprehensive test > rather than "unit test". Yes, that's basically the point I was making. No need to change it right now, although now would be appropriate if you were inclined to. > Although I find it already better than it was, should I extract it into a > separate test? > Actually, I would like to have it as it's now because it covers what we actually > do while the client is communicating with the server. As to the immediate issue, I'd be glad to see separate tests, but I won't even request it. There are two kinds of tests here: testing each element individually and testing the sequences used for communication. Both are valuable. At present we only have the second kind of test. Changing this can wait. The real value of unit tests occurs when you have extensive unit tests available before you find yourself rewriting a class (or writing it the first time, for that matter). Since we have higher priorities right now than rewriting, there's no pressing need to enhance the test suite. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/test... test/CommunicationTest.cpp:123: void SendReceiveTypes(TestedType value, AnotherType anotherTypeValue) On 2015/02/18 14:13:06, sergei wrote: > Before the everything was in one test, would you mind if we make such > improvements in another commit? That's fine. http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/src/... File src/shared/Communication.h (right): http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/src/... src/shared/Communication.h:104: throw new std::runtime_error("Unexpected end of input buffer"); This test for failure should be guarded by the conditional. No sense in testing the buffer if we didn't do an operation on it. http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/test... File test/CommunicationTest.cpp (right): http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/test... test/CommunicationTest.cpp:44: while(!client) There's the possibility of an infinite loop if there's a persistent error in the implementation of the Pipe constructor. There should be a maximum number of iterations; if it reaches the limit, the test should abort. It may be desirable to add a small delay (1-10 ms, say) in case of exception to keep this loop from churning too rapidly. http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/test... test/CommunicationTest.cpp:56: Communication::Pipe server(pipeName, Communication::Pipe::MODE_CREATE); Shouldn't creating the pipe come before connecting to it? If we were seriously improving these tests, we'd have tests for each order of instantiation. That is, isn't this test trying to exercise basic behavior, rather than edge cases? http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/test... test/CommunicationTest.cpp:158: TEST(CommunicationTest, SendReceiveEmptyString) How about 'TypeStringNarrowEmpty'? http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/test... test/CommunicationTest.cpp:174: TEST(CommunicationTest, SendString_ReceiveWString) How about 'TypeStringSendNarrowReceiveWide'? http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/test... test/CommunicationTest.cpp:184: TEST(CommunicationTest, SendWString_ReceiveString) How about 'TypeStringSendWideReceiveNarrow'? http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/test... test/CommunicationTest.cpp:194: TEST(CommunicationTest, SendStrings_ReceiveWStrings) How about 'TypeStringSendNarrowReceiveWide'? http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/test... test/CommunicationTest.cpp:253: TEST(InputOutputBuffersTests, InputOutputBufferEmptyStrings) Consistent naming would start this name and below with "TestStrings..." or "TestStringVector...". I suggest the second form because noticing the difference between "String" and "Strings" inside a InterCaps name is just too slight a difference to be a good name.
P.S. ... Oh, and that one possibility for an infinite loop that needs a fix.
http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... File src/shared/Communication.h (right): http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:99: buffer.read(&value[0], sizeof(std::string::value_type) * length); On 2015/02/18 18:12:47, Eric wrote: > On 2015/02/18 14:13:06, sergei wrote: > > So, why do we need to write suspicious code which violates the > > const-correctness? > > From my point of view, it is _already_ suspicious code. Using const-cast is a > way of documenting the suspicion. I've found it, http://codereview.adblockplus.org/5750789393874944/diff/5629499534213120/src/... . So, everybody is aware about it and we decided that &str[0] is fine as well as it does not do anything which is documented as undefined behavour. I would say it's not suspicious. > > We're relying on the particular implementation of std::string in MSVC. In other > words, we're using something that only looks like standard. My main concern over > this whole issue is (and has been) that we should not do this silently, that is, > without documentation. It should not be a conventional idiom that passes by > without comment. We decided that there are no plans to switch to anything else, we are not using not-working things and, as I understand, MS team does not plan to break std::string. > > I'm beginning to think that the right way to treat this kind of string > initialization is with our own template function. That way we can put all the > documentation in one place and let the use of the function serve as the > point-of-use documentation. I would say it's overengineering here. http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:99: buffer.read(&value[0], sizeof(std::string::value_type) * length); On 2015/02/18 18:12:47, Eric wrote: > On 2015/02/18 14:41:39, sergei wrote: > > From > > http://en.cppreference.com/w/cpp/string/basic_string/data > > From that same page: > "data()[i] == operator[](i) for every i in [0, size()). (until C++11)" > > Given this statement, there is no distinction between &str[0] and str.data(). I > find std.data() a more direct expression, without the legacy burden of C idiom. > You've pointed out that modifying through data() is undefined, which means that > modifying through &str[0] is also undefined; no distinction on that point. Don't mix const methods and non-const methods. On that page they are talking about const methods. As it was mentioned already it's stated on this page that "Modifying the character array accessed through data is undefined behavior". > > There's a proviso that this is only guaranteed before C++11, but Microsoft > doesn't (much) segregate C++11 and C++03 semantics, so we can expect that the > proviso won't matter for us. It's guaranteed not *before* C++11, it's guaranteed *since* C++11. > > Once we all acknowledge that we are willing to write non-standard code for the > sake of expediency (and even efficiency), it will become clearer what to do > about it. What is non-standard and what do we violate here? http://codereview.adblockplus.org/4882650246414336/diff/5629499534213120/src/... src/shared/Communication.h:101: throw new std::runtime_error("Unexpected end of input buffer"); On 2015/02/18 18:12:47, Eric wrote: > A separate issue for this is overkill, but very briefly, if an operation sets > the bad bit it will also set the fail bit, but not vice-versa. > > See http://www.cplusplus.com/reference/ios/basic_ios/bad/ The reason to change it clear, but it's not a single place where we use it, and it's not relevant to the current change, so I would like to have it in another commit. http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/src/... File src/shared/Communication.h (right): http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/src/... src/shared/Communication.h:104: throw new std::runtime_error("Unexpected end of input buffer"); On 2015/02/18 18:12:47, Eric wrote: > This test for failure should be guarded by the conditional. No sense in testing > the buffer if we didn't do an operation on it. moved http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/test... File test/CommunicationTest.cpp (right): http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/test... test/CommunicationTest.cpp:44: while(!client) On 2015/02/18 18:12:47, Eric wrote: > There's the possibility of an infinite loop if there's a persistent error in the > implementation of the Pipe constructor. There should be a maximum number of > iterations; if it reaches the limit, the test should abort. > > It may be desirable to add a small delay (1-10 ms, say) in case of exception to > keep this loop from churning too rapidly. I've added the counter and changed `yeild` by `sleep_for`. On my computer it usually takes two cycles. http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/test... test/CommunicationTest.cpp:56: Communication::Pipe server(pipeName, Communication::Pipe::MODE_CREATE); On 2015/02/18 18:12:47, Eric wrote: > Shouldn't creating the pipe come before connecting to it? If we were seriously > improving these tests, we'd have tests for each order of instantiation. That is, > isn't this test trying to exercise basic behavior, rather than edge cases? This particular test is aimed to exercise the sending of data through the pipe and not connecting. BTW, our bho (client) and engine (server) actually work as in this test. The another order is used in `TEST(CommunicationTest, PipeConnect)`, so I would say there is no reason to create another tests for the present. http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/test... test/CommunicationTest.cpp:158: TEST(CommunicationTest, SendReceiveEmptyString) On 2015/02/18 18:12:47, Eric wrote: > How about 'TypeStringNarrowEmpty'? For this case and below, TypeString is not what we test here, we test that sending and receiving of the specific strings work. Regarding Narrow and Wide, I find the current version to be close to `ToWstring`. Would not it be better to use Utf8 and Utf16 instead? Regarding the underscore, I really find it easier to read and understand with it than without it, but I can remove it of course to be consistent. SendStringReceiveWString vs SendString_ReceiveWString http://codereview.adblockplus.org/4882650246414336/diff/5750085036015616/test... test/CommunicationTest.cpp:253: TEST(InputOutputBuffersTests, InputOutputBufferEmptyStrings) On 2015/02/18 18:12:47, Eric wrote: > Consistent naming would start this name and below with "TestStrings..." or > "TestStringVector...". I suggest the second form because noticing the difference > between "String" and "Strings" inside a InterCaps name is just too slight a > difference to be a good name. We don't start test names with Test. I've fixed 'InputOutputBuffersTests' to 'InputOutputBuffersTest' and removed 'InputOutputBuffer' from test name.
LGTM
Looks fine except for one confusing placement of an assertion in one of the tests. 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) 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' http://codereview.adblockplus.org/4882650246414336/diff/5743114304094208/test... test/CommunicationTest.cpp:58: ASSERT_TRUE(client); This assertion should appear before the declaration of 'clientThread'. If you're going to check it at all and fail the test based on it, you should test it as soon as possible.
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`. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
