Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7492029:
Implement classes used for manipulating D-Bus messages. (Closed)
|
Created:
9 years, 5 months ago by satorux1 Modified:
9 years, 4 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement classes used for manipulating D-Bus messages.
Message/MethodCall/Response classes wrap around DBusMessage.
MessageReader and Message Writer provide API to read and write
D-Bus messages in a type safe fashion.
BUG=90036
TEST=The code is not yet used in Chrome. Run unit tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=94845
Patch Set 1 #
Total comments: 3
Patch Set 2 : remove run_all_unittests.cc from dbus #Patch Set 3 : Add AppendVariantOfByte() etc. Write more tests. #Patch Set 4 : Fix spellings #Patch Set 5 : Use base/format_macros.h #Patch Set 6 : Add AppendArrayOfBytes and tests #
Total comments: 57
Patch Set 7 : address comments #
Total comments: 4
Patch Set 8 : improve ToString() #
Total comments: 6
Patch Set 9 : rework PopArrayOfBytes #Patch Set 10 : Address comments #Patch Set 11 : Add more headers #
Total comments: 31
Patch Set 12 : address comments #Patch Set 13 : minor fix #Patch Set 14 : fix all.gyp #
Total comments: 2
Messages
Total messages: 26 (0 generated)
Drive-by with a testing suggestion. http://codereview.chromium.org/7492029/diff/1/dbus/run_all_unittests.cc File dbus/run_all_unittests.cc (right): http://codereview.chromium.org/7492029/diff/1/dbus/run_all_unittests.cc#newcode8 dbus/run_all_unittests.cc:8: return base::TestSuite(argc, argv).Run(); Do you think it would be possible to just use one of the existing run_all_unittests.cc files? In my opinion having too many of them is hard to maintain.
http://codereview.chromium.org/7492029/diff/1/dbus/run_all_unittests.cc File dbus/run_all_unittests.cc (right): http://codereview.chromium.org/7492029/diff/1/dbus/run_all_unittests.cc#newcode8 dbus/run_all_unittests.cc:8: return base::TestSuite(argc, argv).Run(); On 2011/07/25 18:33:18, Paweł Hajdan Jr. wrote: > Do you think it would be possible to just use one of the existing > run_all_unittests.cc files? In my opinion having too many of them is hard to > maintain. Good point. I found one in base/test. Will check if it works for this.
http://codereview.chromium.org/7492029/diff/1/dbus/run_all_unittests.cc File dbus/run_all_unittests.cc (right): http://codereview.chromium.org/7492029/diff/1/dbus/run_all_unittests.cc#newcode8 dbus/run_all_unittests.cc:8: return base::TestSuite(argc, argv).Run(); On 2011/07/25 19:47:06, satorux1 wrote: > On 2011/07/25 18:33:18, Paweł Hajdan Jr. wrote: > > Do you think it would be possible to just use one of the existing > > run_all_unittests.cc files? In my opinion having too many of them is hard to > > maintain. > > Good point. I found one in base/test. Will check if it works for this. Worked. Removed the file from this patch.
Code I commented in the drive-by LGTM, thanks!
Great stuff. My first round of comments... :) http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode44 dbus/message.cc:44: CHECK(reader->PopByte(&value)) << "Broken message"; I don't think we should force a crash on a failed message parse. Instead we should take a string* as an output argument and return 'false' on error so that the calling code can decide whether to crash or continue. http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode165 dbus/message.cc:165: bool success = false; nit: don't need to initialize success here. http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode213 dbus/message.cc:213: DCHECK(success) << "Unable to allocate memory"; Why not use AppendBasic here? http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode327 dbus/message.cc:327: const uint8* pointer = bytes.data(); I don't think data() is actually a standard member of vector<>? Generally this would just be &bytes[0]. If we provide two interfaces here, one taking a const uint8* + length, then the vector<> version can just call the uint8* version. http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode390 dbus/message.cc:390: // it fails to allocate memory for a byte. We should either pass an error up for all of these so that the calling function knows that it failed and can decide whether to continue or abort, or do a CHECK here and force a crash if we are certain this will only fail on an unrecoverable out of memory error. http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode399 dbus/message.cc:399: CloseContainer(&variant_writer); See my comment in the header about writers for arrays. If we don't want MessageWriter to manage its sub writer, we should at least provide VariantMessageWriter (and ArrayMessageWriter, etc) that call open in the constructor and close on destruction. http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode460 dbus/message.cc:460: *value = tmp_value; // Copy the string. nit: = std::string(tmp_value) is more explicit and makes the comment unnecessary. I assume tmp_value is guaranteed not to be NULL on success? (Need to be certain since std::string(NULL) will crash). http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode468 dbus/message.cc:468: *value = tmp_value; // Copy the string. Same comment as above. http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode560 dbus/message.cc:560: *value = tmp_value; // Copy the string. = std::string(tmp_value) http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode568 dbus/message.cc:568: *value = tmp_value; // Copy the string. = std::string(tmp_value) http://codereview.chromium.org/7492029/diff/12003/dbus/message.h File dbus/message.h (right): http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode37 dbus/message.h:37: }; I'm not sure that there is anything gained by mapping this enum, since we have to include dbus/dbus.h in the header anyway. What happens if a new DBUS_MESSAGE_TYPE is introduced and we cast it to our own enum? http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode58 dbus/message.h:58: }; Same comment as above. http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode73 dbus/message.h:73: void reset_raw_message(DBusMessage* raw_message); Do we need to expose this publicly, or could we make this protected and require that sub-classes set raw_message_? http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode145 dbus/message.h:145: // The following functions do the same for other basic types. This comment is probably not necessary and made me think that Byte was different from Bool, etc. http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode158 dbus/message.h:158: // sub_writer. The client code should close the array by CloseContainer(), |sub_writer| s/by/with/ http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode159 dbus/message.h:159: // once all contents are added. Can multiple arrays use the same sub_writer? If so, does it make sense for MessageWriter to allocate/free the sub_writer? If not, maybe OpenArray() should allocate and return sub_writer, freeing it with CloseContainer()? http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode163 dbus/message.h:163: // then you pass "s" as the signature. need to take "need to take"? http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode172 dbus/message.h:172: // for us to design API that doesn't require the signature but it'll s/it'll/it will/ (it'll is rarely written as a contraction) http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode191 dbus/message.h:191: void AppendArrayOfBytes(const std::vector<uint8>& bytes); This might be more flexible if it takes a const uint8* and a length, instead of a vector. Sometimes binary blobs may not be wrapped up in a vector (and constructing one might introduce an extra copy). http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode198 dbus/message.h:198: // The following functions do the same for other basic types. This comment is probably not necessary and made me think that Byte was different from Bool, etc. http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode231 dbus/message.h:231: // provide type safe API. In the past, there was a Chrome OS blocker bug, s/type safe API/a type safe API http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode244 dbus/message.h:244: // See crosbug.com/7619 for details about the bug in question. Comment about blocker bug is probably not necessary; it should be obvious why type safety is a good thing :) http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode256 dbus/message.h:256: bool HasMore(); nit: HasMoreData() http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode262 dbus/message.h:262: // The following functions do the same for other basic types. nit: Don't separate PopBool,etc from PopByte with a comment. http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode279 dbus/message.h:279: // The following functions do the same for other container types. nit: Don't separate with a comment. http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode287 dbus/message.h:287: // specialized function. I think there should be a version of this that takes a uint8** and a length int* as input and pass ownership of the bytes to the calling function so that it doesn't force an extra copy. (See comment in C++ file) http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode302 dbus/message.h:302: // The following functions do the same for other basic types. nit: Don't separate with a comment.
Steven, Thank you for reviewing the code very carefully and providing great feedback! I think I've addressed most of your points, and answered your comments where I'd rather keep it as before. On 2011/07/27 17:56:25, Steven Bennetts wrote: > Great stuff. My first round of comments... :) > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc > File dbus/message.cc (right): > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode44 > dbus/message.cc:44: CHECK(reader->PopByte(&value)) << "Broken message"; > I don't think we should force a crash on a failed message parse. Instead we > should take a string* as an output argument and return 'false' on error so that > the calling code can decide whether to crash or continue. > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode165 > dbus/message.cc:165: bool success = false; > nit: don't need to initialize success here. > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode213 > dbus/message.cc:213: DCHECK(success) << "Unable to allocate memory"; > Why not use AppendBasic here? > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode327 > dbus/message.cc:327: const uint8* pointer = bytes.data(); > I don't think data() is actually a standard member of vector<>? Generally this > would just be &bytes[0]. > > If we provide two interfaces here, one taking a const uint8* + length, then the > vector<> version can just call the uint8* version. > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode390 > dbus/message.cc:390: // it fails to allocate memory for a byte. > We should either pass an error up for all of these so that the calling function > knows that it failed and can decide whether to continue or abort, or do a CHECK > here and force a crash if we are certain this will only fail on an unrecoverable > out of memory error. > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode399 > dbus/message.cc:399: CloseContainer(&variant_writer); > See my comment in the header about writers for arrays. If we don't want > MessageWriter to manage its sub writer, we should at least provide > VariantMessageWriter (and ArrayMessageWriter, etc) that call open in the > constructor and close on destruction. > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode460 > dbus/message.cc:460: *value = tmp_value; // Copy the string. > nit: = std::string(tmp_value) is more explicit and makes the comment > unnecessary. > I assume tmp_value is guaranteed not to be NULL on success? (Need to be certain > since std::string(NULL) will crash). > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode468 > dbus/message.cc:468: *value = tmp_value; // Copy the string. > Same comment as above. > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode560 > dbus/message.cc:560: *value = tmp_value; // Copy the string. > = std::string(tmp_value) > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.cc#newcode568 > dbus/message.cc:568: *value = tmp_value; // Copy the string. > = std::string(tmp_value) > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h > File dbus/message.h (right): > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode37 > dbus/message.h:37: }; > I'm not sure that there is anything gained by mapping this enum, since we have > to include dbus/dbus.h in the header anyway. What happens if a new > DBUS_MESSAGE_TYPE is introduced and we cast it to our own enum? > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode58 > dbus/message.h:58: }; > Same comment as above. > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode73 > dbus/message.h:73: void reset_raw_message(DBusMessage* raw_message); > Do we need to expose this publicly, or could we make this protected and require > that sub-classes set raw_message_? > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode145 > dbus/message.h:145: // The following functions do the same for other basic > types. > This comment is probably not necessary and made me think that Byte was different > from Bool, etc. > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode158 > dbus/message.h:158: // sub_writer. The client code should close the array by > CloseContainer(), > |sub_writer| > s/by/with/ > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode159 > dbus/message.h:159: // once all contents are added. > Can multiple arrays use the same sub_writer? If so, does it make sense for > MessageWriter to allocate/free the sub_writer? If not, maybe OpenArray() should > allocate and return sub_writer, freeing it with CloseContainer()? > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode163 > dbus/message.h:163: // then you pass "s" as the signature. need to take > "need to take"? > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode172 > dbus/message.h:172: // for us to design API that doesn't require the signature > but it'll > s/it'll/it will/ (it'll is rarely written as a contraction) > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode191 > dbus/message.h:191: void AppendArrayOfBytes(const std::vector<uint8>& bytes); > This might be more flexible if it takes a const uint8* and a length, instead of > a vector. Sometimes binary blobs may not be wrapped up in a vector (and > constructing one might introduce an extra copy). > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode198 > dbus/message.h:198: // The following functions do the same for other basic > types. > This comment is probably not necessary and made me think that Byte was different > from Bool, etc. > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode231 > dbus/message.h:231: // provide type safe API. In the past, there was a Chrome OS > blocker bug, > s/type safe API/a type safe API > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode244 > dbus/message.h:244: // See crosbug.com/7619 for details about the bug in > question. > Comment about blocker bug is probably not necessary; it should be obvious why > type safety is a good thing :) > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode256 > dbus/message.h:256: bool HasMore(); > nit: HasMoreData() > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode262 > dbus/message.h:262: // The following functions do the same for other basic > types. > nit: Don't separate PopBool,etc from PopByte with a comment. > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode279 > dbus/message.h:279: // The following functions do the same for other container > types. > nit: Don't separate with a comment. > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode287 > dbus/message.h:287: // specialized function. > I think there should be a version of this that takes a uint8** and a length int* > as input and pass ownership of the bytes to the calling function so that it > doesn't force an extra copy. (See comment in C++ file) > > http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode302 > dbus/message.h:302: // The following functions do the same for other basic > types. > nit: Don't separate with a comment.
Responses to review comments. http://codereview.chromium.org/7492029/diff/12003/dbus/message.h File dbus/message.h (right): http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode37 dbus/message.h:37: }; On 2011/07/27 21:20:35, satorux1 wrote: > On 2011/07/27 17:56:26, Steven Bennetts wrote: > > I'm not sure that there is anything gained by mapping this enum, since we have > > to include dbus/dbus.h in the header anyway. What happens if a new > > DBUS_MESSAGE_TYPE is introduced and we cast it to our own enum? > > DBUS_MESSAGE_TYPE_INVALID etc. are #define macros. Having an enum type here > makes code a bit more type-safe: > > MessageType GetMessageType() vs. int GetMessageType(). > > These message types are defined in the D-Bus protocol so I don't expect a new > message type will be introduced. Even if it's introduced, it won't affect our > code, as long as we are using our MessageType. > > I added a comment about it. Ah, if these are #defines that makes sense then. Fair enough. http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode73 dbus/message.h:73: void reset_raw_message(DBusMessage* raw_message); On 2011/07/27 21:20:35, satorux1 wrote: > On 2011/07/27 17:56:26, Steven Bennetts wrote: > > Do we need to expose this publicly, or could we make this protected and > require > > that sub-classes set raw_message_? > > Good point. This is exposed for unit tests and ObjectProxy. ObjectProxy needs to > set the response message returned from the server to Response object. We could > make ObjectProxy a friend class, but I guess it's simpler to expose it. I'll have to look at ObjectProxy. Making a test class a friend is standard practice, but we shouldn't do it for ObjectProxy. Unless it makes sense for ObejctProxy to use a derived dbus::Message class, leaving this public is fine, just a little more low level than would be ideal (since it exposes DBusMessage). http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode287 dbus/message.h:287: // specialized function. On 2011/07/27 21:20:35, satorux1 wrote: > On 2011/07/27 17:56:26, Steven Bennetts wrote: > > I think there should be a version of this that takes a uint8** and a length > int* > > as input and pass ownership of the bytes to the calling function so that it > > doesn't force an extra copy. (See comment in C++ file) > > Good point. What about adding such a version when it becomes necessary? > vector<uint8> value can be used for C-ish functions like fwrite(v.data(), 1, > v.size(), fp), so I guess that the uint8** version would be rarely needed. It's not a question of whether or not vector<> can be used, but whether or not it should be used. It tends to introduce a lot of extra copying. For not this is probably fine, just something to consider from a performance perspective.
A few more comments, but generally looks good. I am still not entirely convinced that passing sub_reader/writer around is the best approach. Since a MessageReader/Writer should only have one container open at a time, it seems like it could do the memory management of the reader/writer associated with the container, providing an accessor if necessary. However, I do not feel too strongly about that and would be fine with this as-is. http://codereview.chromium.org/7492029/diff/16001/dbus/message.h File dbus/message.h (right): http://codereview.chromium.org/7492029/diff/16001/dbus/message.h#newcode38 dbus/message.h:38: // (i.e. don't try to cast random integers to this enum). I would remove the last paragraph, I think the first part justifies the use of an enum. We use static_cast<> in the code which will not ensure that the result is a member of the enum (e.g. MessageType foo = static_cast<MessageType>(10) will compile just fine), but as you mentioned, it is unlikely that a new type will be defined, and even if one is it probably won't introduce any errors. http://codereview.chromium.org/7492029/diff/16001/dbus/message.h#newcode183 dbus/message.h:183: // not be a big issue. We can probably omit the above paragraph. While it is well stated, I think the same thoughts will occur to anyone working on this class, and it doesn't help anyone implementing it. Maybe move it into the .cc file if you like. http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc#newcode45 dbus/message.cc:45: return "[broken message]"; use kBrokenMessage for all of these http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc#newcode165 dbus/message.cc:165: return ""; nit: return std::string() http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc#newcode355 dbus/message.cc:355: // a container like AppendArrayOfBytes(str.data(), str.size()); I would put this comment in the header, or omit it entirely. size_t is generally preferred in a C++ api, and this sort of cast to a C API is common enough.
http://codereview.chromium.org/7492029/diff/12003/dbus/message.h File dbus/message.h (right): http://codereview.chromium.org/7492029/diff/12003/dbus/message.h#newcode287 dbus/message.h:287: // specialized function. On 2011/07/27 21:44:13, Steven Bennetts wrote: > On 2011/07/27 21:20:35, satorux1 wrote: > > On 2011/07/27 17:56:26, Steven Bennetts wrote: > > > I think there should be a version of this that takes a uint8** and a length > > int* > > > as input and pass ownership of the bytes to the calling function so that it > > > doesn't force an extra copy. (See comment in C++ file) > > > > Good point. What about adding such a version when it becomes necessary? > > vector<uint8> value can be used for C-ish functions like fwrite(v.data(), 1, > > v.size(), fp), so I guess that the uint8** version would be rarely needed. > > It's not a question of whether or not vector<> can be used, but whether or not > it should be used. It tends to introduce a lot of extra copying. For not this is > probably fine, just something to consider from a performance perspective. I'm convinced. Changed it to uint8* + size_t, to make it consistent with AppendAarrayOfBytes().
BTW, in the patch set 8, I changed ToString() to include message headers such as destination as well.
BTW, in the patch set 8, I changed ToString() to include message headers such as destination as well.
http://codereview.chromium.org/7492029/diff/16001/dbus/message.h File dbus/message.h (right): http://codereview.chromium.org/7492029/diff/16001/dbus/message.h#newcode38 dbus/message.h:38: // (i.e. don't try to cast random integers to this enum). On 2011/07/27 22:08:52, Steven Bennetts wrote: > I would remove the last paragraph, I think the first part justifies the use of > an enum. We use static_cast<> in the code which will not ensure that the result > is a member of the enum (e.g. MessageType foo = static_cast<MessageType>(10) > will compile just fine), but as you mentioned, it is unlikely that a new type > will be defined, and even if one is it probably won't introduce any errors. Done. http://codereview.chromium.org/7492029/diff/16001/dbus/message.h#newcode183 dbus/message.h:183: // not be a big issue. On 2011/07/27 22:08:52, Steven Bennetts wrote: > We can probably omit the above paragraph. While it is well stated, I think the > same thoughts will occur to anyone working on this class, and it doesn't help > anyone implementing it. Maybe move it into the .cc file if you like. Good point. Moved to .cc file so I don't forget about why I designed this way. http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc#newcode45 dbus/message.cc:45: return "[broken message]"; On 2011/07/27 22:08:52, Steven Bennetts wrote: > use kBrokenMessage for all of these Done. http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc#newcode165 dbus/message.cc:165: return ""; On 2011/07/27 22:08:52, Steven Bennetts wrote: > nit: return std::string() "" is used in some other places, and I'd slightly prefer "". http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc#newcode355 dbus/message.cc:355: // a container like AppendArrayOfBytes(str.data(), str.size()); On 2011/07/27 22:08:52, Steven Bennetts wrote: > I would put this comment in the header, or omit it entirely. size_t is generally > preferred in a C++ api, and this sort of cast to a C API is common enough. Removed.
On 2011/07/27 22:08:52, Steven Bennetts wrote: > A few more comments, but generally looks good. > > I am still not entirely convinced that passing sub_reader/writer around is the > best approach. Since a MessageReader/Writer should only have one container open > at a time, it seems like it could do the memory management of the reader/writer > associated with the container, providing an accessor if necessary. However, I do > not feel too strongly about that and would be fine with this as-is. I am also not entirely convinced that MessageReader/Writer is the best approach, but so far I haven't come up with a better one. I was originally adding AppendByte/PopByte, etc. to Message class itself, but got stuck when I started looking at adding support for containers. After some trials and errors, I found that it would be simpler to separate Message and Reader/Writer, just like the underlying dbus library does with DBusMessage and DBusMessageIter. I think we can rework the API if we come up with a better idea. > > http://codereview.chromium.org/7492029/diff/16001/dbus/message.h > File dbus/message.h (right): > > http://codereview.chromium.org/7492029/diff/16001/dbus/message.h#newcode38 > dbus/message.h:38: // (i.e. don't try to cast random integers to this enum). > I would remove the last paragraph, I think the first part justifies the use of > an enum. We use static_cast<> in the code which will not ensure that the result > is a member of the enum (e.g. MessageType foo = static_cast<MessageType>(10) > will compile just fine), but as you mentioned, it is unlikely that a new type > will be defined, and even if one is it probably won't introduce any errors. > > http://codereview.chromium.org/7492029/diff/16001/dbus/message.h#newcode183 > dbus/message.h:183: // not be a big issue. > We can probably omit the above paragraph. While it is well stated, I think the > same thoughts will occur to anyone working on this class, and it doesn't help > anyone implementing it. Maybe move it into the .cc file if you like. > > http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc > File dbus/message.cc (right): > > http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc#newcode45 > dbus/message.cc:45: return "[broken message]"; > use kBrokenMessage for all of these > > http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc#newcode165 > dbus/message.cc:165: return ""; > nit: return std::string() > > http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc#newcode355 > dbus/message.cc:355: // a container like AppendArrayOfBytes(str.data(), > str.size()); > I would put this comment in the header, or omit it entirely. size_t is generally > preferred in a C++ api, and this sort of cast to a C API is common enough.
Steven, please let me know if you have more comments on this. Thanks On Jul 27, 2011 3:36 PM, <satorux@chromium.org> wrote: > On 2011/07/27 22:08:52, Steven Bennetts wrote: >> A few more comments, but generally looks good. >> I am still not entirely convinced that passing sub_reader/writer around >> is the >> best approach. Since a MessageReader/Writer should only have one container > open >> at a time, it seems like it could do the memory management of the > reader/writer >> associated with the container, providing an accessor if necessary. >> However, I > do >> not feel too strongly about that and would be fine with this as-is. > I am also not entirely convinced that MessageReader/Writer is the best > approach, > but so far I haven't come up with a better one. > I was originally adding AppendByte/PopByte, etc. to Message class itself, > but > got stuck when I started looking at adding support for containers. After > some > trials and errors, I found that it would be simpler to separate Message and > Reader/Writer, just like the underlying dbus library does with DBusMessage > and > DBusMessageIter. I think we can rework the API if we come up with a better > idea. >> http://codereview.chromium.org/7492029/diff/16001/dbus/message.h >> File dbus/message.h (right): http://codereview.chromium.org/7492029/diff/16001/dbus/message.h#newcode38 >> dbus/message.h:38: // (i.e. don't try to cast random integers to this >> enum). >> I would remove the last paragraph, I think the first part justifies the >> use of >> an enum. We use static_cast<> in the code which will not ensure that the > result >> is a member of the enum (e.g. MessageType foo = >> static_cast<MessageType>(10) >> will compile just fine), but as you mentioned, it is unlikely that a new >> type >> will be defined, and even if one is it probably won't introduce any >> errors. http://codereview.chromium.org/7492029/diff/16001/dbus/message.h#newcode183 >> dbus/message.h:183: // not be a big issue. >> We can probably omit the above paragraph. While it is well stated, I >> think the >> same thoughts will occur to anyone working on this class, and it doesn't >> help >> anyone implementing it. Maybe move it into the .cc file if you like. >> http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc >> File dbus/message.cc (right): http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc#newcode45 >> dbus/message.cc:45: return "[broken message]"; >> use kBrokenMessage for all of these http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc#newcode165 >> dbus/message.cc:165: return ""; >> nit: return std::string() http://codereview.chromium.org/7492029/diff/19001/dbus/message.cc#newcode355 >> dbus/message.cc:355: // a container like AppendArrayOfBytes(str.data(), >> str.size()); >> I would put this comment in the header, or omit it entirely. size_t is > generally >> preferred in a C++ api, and this sort of cast to a C API is common enough. > http://codereview.chromium.org/7492029/
One last round of mostly nits :) http://codereview.chromium.org/7492029/diff/19012/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/7492029/diff/19012/dbus/message.cc#newcode287 dbus/message.cc:287: // It may make sense to return an error here, as the input string can be Add TODO(satorux) http://codereview.chromium.org/7492029/diff/19012/dbus/message.cc#newcode536 dbus/message.cc:536: reinterpret_cast<int*>(length)); reinterpret_cast should be avoided. It would be better to declare a local 'ilen', pass &ilen to the function, and use static_cast to copy it to *length. http://codereview.chromium.org/7492029/diff/19012/dbus/message.cc#newcode539 dbus/message.cc:539: return true; nit: return (bytes != NULL); http://codereview.chromium.org/7492029/diff/19012/dbus/message.h File dbus/message.h (right): http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode35 dbus/message.h:35: // nit: vs. expample probably unnecessary; hopefully anyone reading this understands that enums are more typesafe than #defines :) http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode45 dbus/message.h:45: // MessageType for why we are redefined data types here. nit: s/redefined/redefining http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode91 dbus/message.h:91: // ... nit: Probably don't need this much detail for a debugging call in the header. Maybe move all but the first two sentences to the C++ file. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode116 dbus/message.h:116: // doesn't need to set this by reset_raw_message(). not: s/by/with http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode139 dbus/message.h:139: // NULL. ObjectProxy will set it properly once it gets a response from This is the only reference here to 'ObjectProxy'. Should probably say something like: 'Derived classes need to set the raw message once a response is received from the server. See object_proxy.h. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode151 dbus/message.h:151: // The data will be written into the given message. 'The data' is unclear. Maybe something like "Data added with Append* will be written to |message|. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode169 dbus/message.h:169: // |sub_writer|. The client code should close the array with s/should/must http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode243 dbus/message.h:243: // writer.AppendString(str); Move this comment (starting with 'The main design goal') above MessageWriter and MessageReader? http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode276 dbus/message.h:276: // will be returned in this case. nit: separate line for return values: Returns true and advances the iterator on success. Returns false if the data type is not an array. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode284 dbus/message.h:284: // advances the iterator and returns true. Arrays of bytes are often nit: Separate line for return value: Returns true and advances the iterator on success. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode290 dbus/message.h:290: // going to use the data after the destruction of the message, Trailing comma. Maybe say something like '|bytes| must be copied of the contents will be referenced after the MessageReader is destroyed.' http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode294 dbus/message.h:294: // success, advances the iterator and returns true. Arrays of object nit: separate line for return value. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode300 dbus/message.h:300: // position. On success, returns true and advances the nit: separate line for return value.
Steven, Thank you for all the feedback! http://codereview.chromium.org/7492029/diff/19012/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/7492029/diff/19012/dbus/message.cc#newcode287 dbus/message.cc:287: // It may make sense to return an error here, as the input string can be On 2011/07/29 20:27:12, Steven Bennetts wrote: > Add TODO(satorux) Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.cc#newcode536 dbus/message.cc:536: reinterpret_cast<int*>(length)); On 2011/07/29 20:27:12, Steven Bennetts wrote: > reinterpret_cast should be avoided. It would be better to declare a local > 'ilen', pass &ilen to the function, and use static_cast to copy it to *length. Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.cc#newcode539 dbus/message.cc:539: return true; On 2011/07/29 20:27:12, Steven Bennetts wrote: > nit: return (bytes != NULL); Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h File dbus/message.h (right): http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode35 dbus/message.h:35: // On 2011/07/29 20:27:12, Steven Bennetts wrote: > nit: vs. expample probably unnecessary; hopefully anyone reading this > understands that enums are more typesafe than #defines :) Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode45 dbus/message.h:45: // MessageType for why we are redefined data types here. On 2011/07/29 20:27:12, Steven Bennetts wrote: > nit: s/redefined/redefining Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode91 dbus/message.h:91: // ... On 2011/07/29 20:27:12, Steven Bennetts wrote: > nit: Probably don't need this much detail for a debugging call in the header. > Maybe move all but the first two sentences to the C++ file. Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode116 dbus/message.h:116: // doesn't need to set this by reset_raw_message(). On 2011/07/29 20:27:12, Steven Bennetts wrote: > not: s/by/with Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode139 dbus/message.h:139: // NULL. ObjectProxy will set it properly once it gets a response from On 2011/07/29 20:27:12, Steven Bennetts wrote: > This is the only reference here to 'ObjectProxy'. Should probably say something > like: 'Derived classes need to set the raw message once a response is received > from the server. See object_proxy.h. Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode151 dbus/message.h:151: // The data will be written into the given message. On 2011/07/29 20:27:12, Steven Bennetts wrote: > 'The data' is unclear. Maybe something like "Data added with Append* will be > written to |message|. Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode169 dbus/message.h:169: // |sub_writer|. The client code should close the array with On 2011/07/29 20:27:12, Steven Bennetts wrote: > s/should/must Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode243 dbus/message.h:243: // writer.AppendString(str); On 2011/07/29 20:27:12, Steven Bennetts wrote: > Move this comment (starting with 'The main design goal') above MessageWriter and > MessageReader? Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode276 dbus/message.h:276: // will be returned in this case. On 2011/07/29 20:27:12, Steven Bennetts wrote: > nit: separate line for return values: > Returns true and advances the iterator on success. > Returns false if the data type is not an array. Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode290 dbus/message.h:290: // going to use the data after the destruction of the message, On 2011/07/29 20:27:12, Steven Bennetts wrote: > Trailing comma. Maybe say something like '|bytes| must be copied of the contents > will be referenced after the MessageReader is destroyed.' Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode294 dbus/message.h:294: // success, advances the iterator and returns true. Arrays of object On 2011/07/29 20:27:12, Steven Bennetts wrote: > nit: separate line for return value. Done. http://codereview.chromium.org/7492029/diff/19012/dbus/message.h#newcode300 dbus/message.h:300: // position. On success, returns true and advances the On 2011/07/29 20:27:12, Steven Bennetts wrote: > nit: separate line for return value. Done.
LGTM!
Change committed as 94845
Compile failure! http://build.chromium.org/p/chromium/builders/Linux/builds/10288/steps/compil... cc1plus: warnings being treated as errors dbus/message_unittest.cc:255: error: integer constant is too large for 'long' type dbus/message_unittest.cc:325: error: integer constant is too large for 'long' type dbus/message_unittest.cc:325: error: integer constant is too large for 'long' type make: *** [out/Release/obj.target/dbus_unittests/dbus/message_unittest.o] Error 1 http://codereview.chromium.org/7492029/diff/32001/dbus/message_unittest.cc File dbus/message_unittest.cc (right): http://codereview.chromium.org/7492029/diff/32001/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:255: dict_entry_writer.AppendInt64(1234567890123456789); Use GG_INT64_C(1234567890123456789) (base/port.h, but don’t include that, include base/basictypes.h) or choose a smaller number. http://codereview.chromium.org/7492029/diff/32001/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:325: EXPECT_EQ(1234567890123456789, int64_value); Same.
Landed fix in http://crrev.com/94846
Oops, sorry about it, and thank you for the immediate fix!! On Sat, Jul 30, 2011 at 1:38 PM, <rsleevi@chromium.org> wrote: > Landed fix in http://crrev.com/94846 > http://codereview.chromium.**org/7492029/<http://codereview.chromium.org/7492...
I'm puzzled. This failure wasn't caught by Linux trybot and the commit queue. My local linux build was also fine. I'm wondering why it broke the linux build bot. On Sat, Jul 30, 2011 at 3:52 PM, Satoru Takabayashi <satorux@chromium.org>wrote: > Oops, sorry about it, and thank you for the immediate fix!! > On Sat, Jul 30, 2011 at 1:38 PM, <rsleevi@chromium.org> wrote: >> Landed fix in http://crrev.com/94846 >> http://codereview.chromium.**org/7492029/<http://codereview.chromium.org/7492...
On 2011/07/30 23:08:02, satorux1 wrote: > I'm puzzled. This failure wasn't caught by Linux trybot and the commit > queue. My local linux build was also fine. I'm wondering why it broke the > linux build bot. The Linux clobber builder builds 'all'. The other bots build specific subsets, IIRC. I'm guessing the Linux clobber builder is the only one building your dbus/ unit tests (possibly the only one building your dbus target). You can confirm by looking under the master.tryserver and master.chromium configs
On Sat, Jul 30, 2011 at 4:09 PM, <rsleevi@chromium.org> wrote: > On 2011/07/30 23:08:02, satorux1 wrote: >> I'm puzzled. This failure wasn't caught by Linux trybot and the commit >> queue. My local linux build was also fine. I'm wondering why it broke the >> linux build bot. > The Linux clobber builder builds 'all'. The other bots build specific > subsets, > IIRC. > I'm guessing the Linux clobber builder is the only one building your dbus/ > unit > tests (possibly the only one building your dbus target). You can confirm > by > looking under the master.tryserver and master.chromium configs The linux trybot built dbus_unittests: http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/40178/st... CXX(target) out/Debug/obj.target/dbus_unittests/base/test/run_all_unittests.o CXX(target) out/Debug/obj.target/dbus_unittests/dbus/message_unittest.o LINK(target) out/Debug/dbus_unittests LINK(target) out/Debug/dbus_unittests: Finished Anyway, thank you for the fix. I'll be more careful about using large integer literals.
The master.chromium Linux clobber builder runs Lucid, 32 bits The 'linux' trybot runs Lucid, 64 bits . Looking at http://codesearch.google.com/codesearch#OAMlx_jo-ck/tools/build/masters/maste... , I don't see any 32-bit Linux builders (unless I'm misreading) On the main build tree, http://codesearch.google.com/codesearch#OAMlx_jo-ck/tools/build/masters/maste... , the Linux builder slaves that are 32-bit are [ 'Linux Builder (dbg)', 'Linux' ] Of those, 'Linux' builds all, while 'Linux Builder (dbg)' only builds a specific list of targets - http://codesearch.google.com/codesearch#OAMlx_jo-ck/tools/build/masters/maste... May be worth filing an infra issue for a Linux x32 trybot. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
