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: (8 comments) I think the main point of contention is over what to do in situations where a local variable is uninitialized and we think we can do a better job than the compiler of proving that the next read of that local variable cannot take place unless a function along the way writes to that variable. As I see it, we have three options each time this comes up: 1. Double down by suppressing the warning (see https://stackoverflow.com/questions/5080848/disable-gcc-may-be-used-uninitialized-on-a-particular-variable/5080980#5080980). 2. Allow that we might be wrong and provide a default value that will cause the process to crash. 3. Allow that we might be wrong and provide a default value that will cause the process to behave as correctly as possible. I generally prefer #1 because I'm really confident in my ability to look at a function and determine that it'll always set a variable passed in by pointer if it returns Status::OK(). Philosophically I'm also not opposed to #2 because the failure mode is obvious and loud enough that we'll notice and correct our faulty assumption. However, I'm concerned about #3: it's sometimes hard to reason about exactly what effect a default value will have on a piece of code, and if the effect of the default value isn't "loud" enough, we might not even notice that one of our assumptions was wrong. 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; > 1.compilation warning: Ideally I'd say we should CHECK() and crash if cur_elem wasn't set, but that means an extra branch on every iteration of the loop, and this is something of a hot path. If we pretend that we managed to reach L384 without setting cur_elem, an uninitialized value _is_ worse than 0 in that the subsequent behavior varies with each iteration (because the uninitialized value could be anything). So I guess I agree that a default value of 0 is an improvement. http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: PS1: > i changed to gcc8.2.1, and the warning info is still there: In this case I think initialization to nullptr is OK, because it'll at least SIGSEGV and crash if we tried to read the map, which would signal that something is wrong. 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)); > /mnt/ddb/2/helif/apache/kudu/src/kudu/consensus/leader_election.cc: In memb This is more concerning; if we were somehow wrong, elections may fail. That's a really non-obvious failure mode. Given that, and given that it's not possible to test whether 'decision' was or wasn't set after GetDecision(), I'd vote for suppression here. 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: > In file included from /mnt/ddb/2/helif/apache/kudu/src/kudu/util/env.h:26:0 This is test code so I don't really care; a nullptr is fine I guess. http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/hms/hms_client-test.cc File src/kudu/hms/hms_client-test.cc: PS1: > /mnt/ddb/2/helif/apache/kudu/src/kudu/hms/hms_client-test.cc: In member fun That's totally bizarre; there are tests for 'protection' in the same function, before those dereferences. Can we just suppress the warning in this case? 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 boost::none after FindRow() is a valid value. 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 may be optional in a clearer way than a magic -1 value does. Could you do the 'auto prev_row_idx = boost::make_optional(...)' trick here? Or just suppress the warning? 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? -- 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: Sat, 02 Feb 2019 19:35:21 +0000 Gerrit-HasComments: Yes
