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 <hzhel...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hzhel...@corp.netease.com>
Gerrit-Comment-Date: Wed, 13 Feb 2019 12:05:45 +0000
Gerrit-HasComments: Yes

Reply via email to