My biggest question is whether 'offset' args should be uint64/"size64_t". See comment in buffer_blob.cc for details. Food for thought....
======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/gears/base/common/byte_store.cc?a=1 File //depot/googleclient/gears/opensource/gears/base/common/byte_store.cc (snapshot 1) ------------------------------------ Line 347: if (read_buffer_.Size() < static_cast<int64>(buffer_size)) { I'm confused -- buffer_size is already an int64. Why have the casts here? Or did you mean to cast .Size() up to <int64>? ======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/gears/base/firefox/install.rdf.m4?a=1 File //depot/googleclient/gears/opensource/gears/base/firefox/install.rdf.m4 (snapshot 1) ------------------------------------ Line 83: <em:targetPlatform>Linux_x86_64-gcc3</em:targetPlatform> I wonder if this should set <targetPlatform> too, for consistency? If there was some good reason not to, that's fine too. ======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/gears/blob/blob_test.cc?a=1 File //depot/googleclient/gears/opensource/gears/blob/blob_test.cc (snapshot 1) ------------------------------------ Line 302: TEST_ASSERT(static_cast<size_t>(blob->Length()) == strlen(data1)); This diff goes away if you return size64_t/uint64 for Length() -- see comment below. ======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/gears/blob/buffer_blob.cc?a=1 File //depot/googleclient/gears/opensource/gears/blob/buffer_blob.cc (snapshot 1) ------------------------------------ Line 41: assert(0 <= num_bytes && num_bytes <= kint32max); "num_bytes >= 0 && num_bytes <=" would be clearer, I think. Ditto below with "num_bytes" and "buffer_size". ------------------------------------ Line 45: int64 BufferBlob::Read(uint8 *destination, int64 offset, So a bunch of these Blob functions use "int64", when logically they are size_t style arguments. You obviously can't use size_t, as it would change the behavior in 32-bit compiles. But what you sort of want is a forced-64-bit size_t. ("size64_t" ?) This seems like a valid argument for using uint64 for these args, even though it's otherwise frowned upon by Google style. And after doing that, many of these Blob diffs could be reverted, I think? ------------------------------------ Line 50: int64 buffer_size = static_cast<int64>(buffer_.size()); Making buffer_size be a size_t seems a little closer to the intent. ------------------------------------ Line 51: assert(0 <= buffer_size && buffer_size <= kint32max); This looks like a new constraint. buffer_.size() should be allowed to be greater than kint32max, shouldn't it? Otherwise, why have int64 params to all Blob functions? ------------------------------------ Line 58: assert(0 <= num_bytes && num_bytes <= kint32max); New constraint?? ------------------------------------ Line 70: int64 buffer_size = static_cast<int64>(buffer_.size()); A lot of this goes away if the args are size64_t/uint64? ------------------------------------ Line 93: assert(0 <= buffer_size && buffer_size <= kint32max); New constraint? ------------------------------------ Line 101: assert(0 <= buffer_size && buffer_size <= kint32max); New constraint? ======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/gears/canvas/blob_backed_skia_input_stream.cc?a=1 File //depot/googleclient/gears/opensource/gears/canvas/blob_backed_skia_input_stream.cc (snapshot 1) ------------------------------------ Line 46: int64 length = blob_->Length(); If Length() returned uint64, I think this goes away... ------------------------------------ Line 55: int64 remaining = blob_->Length() - blob_offset_; ...and this becomes a comparison of Length() and blob_offset_. ======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/gears/tools/config.mk?a=1 File //depot/googleclient/gears/opensource/gears/tools/config.mk (snapshot 1) ------------------------------------ Line 123: ifeq ($(ARCH),) Oh, I see, this only takes effect for (OS,linux). The lack of indentation makes this hard to grok. Consider indenting or adding a comment. ======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/gears/tools/rules.mk?a=1 File //depot/googleclient/gears/opensource/gears/tools/rules.mk (snapshot 1) ------------------------------------ Line 220: # we created merged installers Nit: add trailing period on sentence. ======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/third_party/gecko_1.9/linux/gecko_sdk/lib64/libxpcom.so?a= File //depot/googleclient/gears/opensource/third_party/gecko_1.9/linux/gecko_sdk/lib64/libxpcom.so ------------------------------------ > The binary libraries (e.g. libxpcom.so, libxul.so) that are new > to third_party are copied straight from Ubuntu's /usr/lib64/firefox. Does Mozilla distribute a version of these? Using libs from a mozilla URL, instead of a local path, feels more "right" to me. ======================================================================== -- To respond, reply to this email or visit http://mondrian.corp.google.com/10624727
