Henry Robinson has posted comments on this change. Change subject: IMPALA-5389: simplify BufferDescriptor lifetime ......................................................................
Patch Set 1: Code-Review+1 (5 comments) This does seem more understandable to me. Just some small suggestions. http://gerrit.cloudera.org:8080/#/c/7182/1/be/src/runtime/disk-io-mgr-scan-range.cc File be/src/runtime/disk-io-mgr-scan-range.cc: Line 85: *buffer = nullptr; maybe prefer buffer->reset() here (for me it's clearer not to use the overloaded operator). http://gerrit.cloudera.org:8080/#/c/7182/1/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: PS1, Line 208: len_(0), : eosr_(false), : scan_range_offset_(0 we've been moving towards initializing members with constants in the header file. If, like me, you prefer that style suggest you do so here to keep the initializer list short. PS1, Line 700: unique_ptr<BufferDescriptor> make_unique(this, reader, range, buffer, buffer_size, reader->mem_tracker_)? PS1, Line 710: std remove std::? http://gerrit.cloudera.org:8080/#/c/7182/1/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: PS1, Line 217: std::unique_ptr<DiskIoMgr::BufferDescriptor>&& buffer > I believe this can be std::unique_ptr<DiskIoMgr::BufferDescriptor>, since w Right, since you can't pass a unique_ptr<> except by moving it, I think pass-by-value is equivalent. There's an argument in favour of keeping this an rvalue ref though, because if we ever changed the type of the ptr to be copyable, pass-by-value would silently start copying at each callsite - it's the sort of latent bug that could go undetected. From a readability perspective, rvalue-refs are explicit that the argument is going to get consumed. I don't feel very strongly though. It would be good to be consistent, since there are other methods that take a unique_ptr by value. -- To view, visit http://gerrit.cloudera.org:8080/7182 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I007d098e9a1abb1f684be37b7f1ee6c03d3879b2 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-HasComments: Yes
