Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8127 )
Change subject: WIP: Pull StatusOr<T> and related tests from protobuf ...................................................................... Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/8127/1/src/kudu/util/statusor-test.cc File src/kudu/util/statusor-test.cc: http://gerrit.cloudera.org:8080/#/c/8127/1/src/kudu/util/statusor-test.cc@267 PS1, Line 267: any test for a non-copyable object such as unique_ptr? http://gerrit.cloudera.org:8080/#/c/8127/1/src/kudu/util/statusor.h File src/kudu/util/statusor.h: http://gerrit.cloudera.org:8080/#/c/8127/1/src/kudu/util/statusor.h@11 PS1, Line 11: // * Redistributions in binary form must reproduce the above : // copyright notice, this list of conditions and the following disclaimer : // in the documentation and/or other materials provided with the : // distribution. need to update top-level LICENSE.txt to centralize the copyright notice http://gerrit.cloudera.org:8080/#/c/8127/1/src/kudu/util/statusor.h@98 PS1, Line 98: UNKNOWN do we have such a thing? http://gerrit.cloudera.org:8080/#/c/8127/1/src/kudu/util/statusor.h@110 PS1, Line 110: PosixErrorSpace::EINVAL this doesn't exist in kudu either http://gerrit.cloudera.org:8080/#/c/8127/1/src/kudu/util/statusor.h@124 PS1, Line 124: const T& do we need a move-constructor T&& value as well? It seems like that would be necessary to be able to have a StatusOr<unique_ptr<Foo>> http://gerrit.cloudera.org:8080/#/c/8127/1/src/kudu/util/statusor.h@134 PS1, Line 134: StatusOr& operator=(const StatusOr& other); should we have a move-assignment operator as well? http://gerrit.cloudera.org:8080/#/c/8127/1/src/kudu/util/statusor.h@191 PS1, Line 191: const Status& status should this pass by value so we can move-assign the status into status_? http://gerrit.cloudera.org:8080/#/c/8127/1/src/kudu/util/statusor.h@193 PS1, Line 193: status_ = Status::InvalidArgument("Status::OK is not a valid argument."); should these have DCHECKs involved? I think these error messages are likely to be confusing as well, since you'd think that you called a method and passed an invalid argument, but really the issue was internal to that method having to do with its return value. Maybe RuntimeError is more appropriate http://gerrit.cloudera.org:8080/#/c/8127/1/src/kudu/util/statusor.h@200 PS1, Line 200: inline StatusOr<T>::StatusOr(const T& value) { same -- To view, visit http://gerrit.cloudera.org:8080/8127 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If2d86a3769da733456956fb892de6d679a011a63 Gerrit-Change-Number: 8127 Gerrit-PatchSet: 1 Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 22 Sep 2017 19:37:10 +0000 Gerrit-HasComments: Yes
