helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12294 )
Change subject: Fix compilation warnings under debian8.9 ...................................................................... Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/cfile/rle_block.h File src/kudu/cfile/rle_block.h: http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/cfile/rle_block.h@380 PS1, Line 380: CppType cur_elem = 0; > Ideally I'd say we should CHECK() and crash if cur_elem wasn't set, but tha Done. In addition, the reason why don't put the 'cur_elem' out of the loop is that the worry your mentioned previously. http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: PS1: > In this case I think initialization to nullptr is OK, because it'll at leas Done http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/consensus/leader_election.cc File src/kudu/consensus/leader_election.cc: http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/consensus/leader_election.cc@304 PS1, Line 304: CHECK_OK(vote_counter_->GetDecision(&decision)); > This is more concerning; if we were somehow wrong, elections may fail. That Done http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/fs/log_block_manager-test-util.cc File src/kudu/fs/log_block_manager-test-util.cc: PS1: > This is test code so I don't really care; a nullptr is fine I guess. Done http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/hms/hms_client-test.cc File src/kudu/hms/hms_client-test.cc: PS1: > That's totally bizarre; there are tests for 'protection' in the same functi i think it will be much more acceptable through 'boost::make_optional' to initialize the variable(boost::none). http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/integration-tests/log_verifier.cc File src/kudu/integration-tests/log_verifier.cc: http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/integration-tests/log_verifier.cc@151 PS1, Line 151: int64_t expected_term = kNotOnThisServer; > 1.the original warning is: it seems boost::make_optional also works :) http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/tablet/cfile_set.cc@320 PS1, Line 320: boost::optional<rowid_t> opt_rowid(0); > This is a hot path; I'd prefer to suppress the warning. Especially because all right, I gave up on fixing it http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/tablet/tablet-test-util.h File src/kudu/tablet/tablet-test-util.h: http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/tablet/tablet-test-util.h@661 PS1, Line 661: int64_t prev_row_idx = -1; > boost::optional here is semantically useful; it indicates that something ma yep, it works:) http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/util/status.cc File src/kudu/util/status.cc: http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/util/status.cc@49 PS1, Line 49: const char* type = nullptr; > Can we add a default case with a LOG(FATAL) instead? Done -- To view, visit http://gerrit.cloudera.org:8080/12294 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2464ff7938f79d090ebd1676da2108ce7dae4ca5 Gerrit-Change-Number: 12294 Gerrit-PatchSet: 1 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Wed, 13 Feb 2019 12:05:45 +0000 Gerrit-HasComments: Yes
