LGTM All right, your call. Consider adding some comments to explain the intent of the casts + range checks.
On Tue, Mar 31, 2009 at 12:05 AM, Nigel Tao <[email protected]> wrote: > On Tue, Mar 31, 2009 at 17:27, Chris Prince <[email protected]> wrote: >> My specific concern is that the new code is doing sanity checks, during >> conversion, that are pretty non-obvious to me. If these continue to be >> scattered through the code, somebody is going to forget to do them at some >> point. > > People might not forget to do so, given that leaving out a static_cast > is a compile error (on 64-bit Ubuntu Hardy). True, we don't have a > 64-bit Ubuntu build-bot yet, but getting to build once is the first > step to getting it to build continuously. > > >> If you had conversion functions or sanity-checking functions, which hid the >> details from callers, that would alleviate my concern. >> >> You're a better judge of what minimal set of operations you would need. But >> some examples might be: >> * int64 CastToBlobSize(size_t) // returns -1 on error >> * int BlobSizeCompare(size_t size1, int64 size2) // returns >> negative/zero/pos >> And these would hide all the error checking / assertions. > > I think that the size_t versus int64 issue only really bites us for > BufferBlobs, not for any other sorts of Blobs. BufferBlobs use a > std::vector, which is size_t, whereas other Blobs generally speak > int64. I'm not sure if adding a functional layer of indirection just > for buffer_blob.cc would gain much, if anything, since buffer_blob.cc > is a small file (109 lines of code, including a copyright header), and > e.g. CastToBlobSize would just be an inline static_cast. >
