Michael Ho has posted comments on this change. Change subject: IMPALA-3854: Fix use-after-free in HdfsTextScanner::Close() ......................................................................
Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/3630/3/be/src/exec/scanner-context.cc File be/src/exec/scanner-context.cc: Line 69: std::unique_ptr<Stream> stream(new Stream(this)); > I think it may be better to have a regular pointer here to avoid implicit d As discussed offline, the unique_ptr here shows the ownership of the stream by the ScannerContext. Whether we should use implicit destruction is a bit beyond this change IMHO. To certain extent, it's the same as asking whether we should use scoped_ptr (or the like) in our code. FWIW, we have plenty of places in our code which relies on implicit destruction to free resources (e.g. RuntimeState's object pool, various classes' scoped_ptr). We don't always explicitly destroy those resources in the destructors. Whether those are good practices can be discussed but I prefer to stick to the existing convention in this patch for consistency. -- To view, visit http://gerrit.cloudera.org:8080/3630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia88f6285563ff669ae215af22ed2d45e5398adae Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-HasComments: Yes
