Todd Lipcon has posted comments on this change. Change subject: KUDU-1259: new scanner API with an encapsulated Batch object ......................................................................
Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/1562/4/src/kudu/client/client.h File src/kudu/client/client.h: Line 1051: // for (KuduRowResult row : batch) { > does that work even though *it returns a temporary rather than a reference? Left this as is, since a reference to a temporary seems wrong, and the KuduRowResult has a trivial copy constructor so it should be optimizable anyway. Line 1067: class KUDU_EXPORT KuduScanBatch { > If you intend to follow the C++ container style, it would be great to defin Done http://gerrit.cloudera.org:8080/#/c/1562/4/src/kudu/client/row_result.h File src/kudu/client/row_result.h: Line 38: w > Circling back to my other comment about an inline class. Since this class i I like this idea. Made the rename, and left a typedef of the old name to preserve API compatibility. It breaks the ABI, but I think that's OK given the beta status. Line 89: friend class KuduScanBatch; > Good idea... let me see if this is possible while also retaining API compat I did this. However, the friend declaration is still necessary -- inner classes don't get special access to their enclosing class in C++03. I believe C++11 changes this. -- To view, visit http://gerrit.cloudera.org:8080/1562 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I29fd4fbb8b906ffa591853ab625ac4b089da4bc9 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Martin Grund <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
