Adar Dembo has posted comments on this change. Change subject: Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/8077/3/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: Line 238: std::array<const Slice, 2> slices {{ mal, header }}; Why do you need to use std::array in some cases and a regular array in others? PS3, Line 475: if (has_checksums() && FLAGS_cfile_verify_checksums) { : results = ArrayView<Slice>(results_backing.data(), 2); : } else { : results = ArrayView<Slice>(results_backing.data(), 1); : } Would a ternary simplify this? http://gerrit.cloudera.org:8080/#/c/8077/3/src/kudu/util/array_view.h File src/kudu/util/array_view.h: Line 2: * Copyright 2015 The WebRTC Project Authors. All rights reserved. Need to update our LICENSE file? Line 17: // Many functions read from or write to arrays. The obvious way to do this is Would be useful to include a comment on the provenance of ArrayView, and perhaps a word or two on how it was modified for Kudu (if at all). Line 57: // Contains17(kudu::ArrayView<int>(arr, size)); // pointer + size Nit: indentation on the inline comment. Line 77: template <typename U> For containers like these, why do the constructors wind up templatized on a different type than the class's template type? Is it because declaring it this way lets the constructor be used both when U == T and when U != T? PS3, Line 85: U (&array)[N] Can't say I've seen this syntax before. It means "array of N elements of type U? -- To view, visit http://gerrit.cloudera.org:8080/8077 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
