Todd Lipcon 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 othe at first I was excited about being C++y, but later decided that C arrays were easier to read anyway. Will switch to C arrays throughout. 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? Done 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? done. I also filed https://issues.apache.org/jira/browse/LEGAL-330 to double-check that the PATENTS file is not a problem. 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 pe done (in the file header) Line 57: // Contains17(kudu::ArrayView<int>(arr, size)); // pointer + size > Nit: indentation on the inline comment. Done Line 77: template <typename U> > For containers like these, why do the constructors wind up templatized on a right, so long as the pointer types are implicitly convertible. eg if you have ArrayView<FooBase*> and you have a FooDerivedClass*, then that up-cast is implicitly convertible and so this constructor will be used. Same with converting from T* to const T*, etc. PS3, Line 85: U (&array)[N] > Can't say I've seen this syntax before. It means "array of N elements of ty I think it's a "a reference to an array of N elements of type U". This is just copied code. -- 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-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
