Adar Dembo 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) I started to review this but stopped because it looks like the vast majority of these issues boil down into two major categories: 1. An uninitialized variable that is passed by pointer into a function. The caller only reads the variable if the function returns success, and the function only writes the variable if it returns success. 2. An uninitialized variable that is set by a switch's case statement. It's guaranteed that one of the case statements is reached. For #1, you could consider using https://stackoverflow.com/a/5080980 instead. For #2, a default case statement with a LOG(FATAL) or equivalent should do the trick. http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/cfile/bshuf_block.cc File src/kudu/cfile/bshuf_block.cc: http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/cfile/bshuf_block.cc@99 PS1, Line 99: uint32_t mid_key = 0; In this case, I think size_of_elem_ must be either 1, 2, or 4. Would a default case statement that did a LOG(FATAL) or some such pacify gcc? 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; I'm skeptical of this: sure the previous uninitialized value is harmful, but what makes 0 more useful? What will the effect on the seek be? There is one code path within rle_decoder_.Get() that doesn't set cur_elem_ but still returns true...but even that code path has a DCHECK() that should prevent that from happening in debug builds. http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: PS1: The changes here shouldn't be necessary; it's not possible for ReadIssetBitmap or ReadNullBitmap to return success without setting the bitmaps. Why doesn't gcc see that? Does it only warn in old versions of gcc? 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)); Likewise, decision will always be set if this returns success. http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/consensus/log-test.cc File src/kudu/consensus/log-test.cc: http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/consensus/log-test.cc@331 PS1, Line 331: int offset = 0; Could we use a default case with LOG(FATAL) instead of this? 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: Same feedback. http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/hms/hms_client-test.cc File src/kudu/hms/hms_client-test.cc: PS1: What was the issue here? 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; What was the warning here exactly? I thought the default constructor for boost::optional sets the variable to boost::none? http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/rpc/server_negotiation.cc@703 PS1, Line 703: security::VerificationResult res = security::VerificationResult::VALID; Could you use a default case statement? -- 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-Comment-Date: Tue, 29 Jan 2019 19:21:30 +0000 Gerrit-HasComments: Yes
