You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2016/10/03 18:53:54 UTC
[kudu-CR] Clean up switch statements in column predicate.cc
Hello Dinesh Bhat, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4599
to review the following change.
Change subject: Clean up switch statements in column_predicate.cc
......................................................................
Clean up switch statements in column_predicate.cc
Where possible, remove default branches in favor of LOG(FATAL) calls
outside the switch. This makes it more likely that the switch will be
updated in the future for new predicate types, since C++ compilers can
warn on incomplete switches if there is no default branch.
Also cleans up a confusing fall-through in ColumnPredicate::operator==.
Change-Id: I80b750f7bc7455607c380ca65744d8cc66eeabd1
---
M src/kudu/common/column_predicate.cc
1 file changed, 5 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/4599/1
--
To view, visit http://gerrit.cloudera.org:8080/4599
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I80b750f7bc7455607c380ca65744d8cc66eeabd1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Clean up switch statements in column predicate.cc
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Clean up switch statements in column_predicate.cc
......................................................................
Patch Set 1: Code-Review+2
OK, though I think Dinesh had added default switch statements to remove some "return without value" (or whatever) warnings in this code. Can those still be addressed?
--
To view, visit http://gerrit.cloudera.org:8080/4599
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I80b750f7bc7455607c380ca65744d8cc66eeabd1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Clean up switch statements in column predicate.cc
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Clean up switch statements in column_predicate.cc
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4599
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I80b750f7bc7455607c380ca65744d8cc66eeabd1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Clean up switch statements in column predicate.cc
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: Clean up switch statements in column_predicate.cc
......................................................................
Clean up switch statements in column_predicate.cc
Where possible, remove default branches in favor of LOG(FATAL) calls
outside the switch. This makes it more likely that the switch will be
updated in the future for new predicate types, since C++ compilers can
warn on incomplete switches if there is no default branch.
Also cleans up a confusing fall-through in ColumnPredicate::operator==.
Change-Id: I80b750f7bc7455607c380ca65744d8cc66eeabd1
Reviewed-on: http://gerrit.cloudera.org:8080/4599
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Dinesh Bhat <di...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/common/column_predicate.cc
1 file changed, 5 insertions(+), 3 deletions(-)
Approvals:
Dinesh Bhat: Looks good to me, but someone else must approve
Adar Dembo: Looks good to me, approved
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4599
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I80b750f7bc7455607c380ca65744d8cc66eeabd1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Clean up switch statements in column predicate.cc
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Clean up switch statements in column_predicate.cc
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4599/1/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
Line 592: LOG(FATAL) << "unknown predicate type";
> Please confirm that this doesn't return a warning about not returning a boo
It doesn't; we use this pattern in multiple places already in this file and elsewhere.
--
To view, visit http://gerrit.cloudera.org:8080/4599
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I80b750f7bc7455607c380ca65744d8cc66eeabd1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Clean up switch statements in column predicate.cc
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: Clean up switch statements in column_predicate.cc
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4599/1/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
Line 592: LOG(FATAL) << "unknown predicate type";
Please confirm that this doesn't return a warning about not returning a bool. Otherwise LGTM.
--
To view, visit http://gerrit.cloudera.org:8080/4599
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I80b750f7bc7455607c380ca65744d8cc66eeabd1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes