impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5389: simplify BufferDescriptor lifetime
Date Thu, 15 Jun 2017 00:06:35 GMT
Henry Robinson has posted comments on this change.

Change subject: IMPALA-5389: simplify BufferDescriptor lifetime

Patch Set 1: Code-Review+1


This does seem more understandable to me. Just some small suggestions.
File be/src/runtime/

Line 85:   *buffer = nullptr;
maybe prefer buffer->reset() here (for me it's clearer not to use the overloaded operator).
File be/src/runtime/

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::?
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I007d098e9a1abb1f684be37b7f1ee6c03d3879b2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Joe McDonnell <>
Gerrit-HasComments: Yes

View raw message