Jim Apple has posted comments on this change. Change subject: PREVIEW: IMPALA-2615: support [[nodiscard]] on Status ......................................................................
Patch Set 5: (14 comments) Halfway through reviewing; though we should hash some of this out before I do the second half. http://gerrit.cloudera.org:8080/#/c/7253/5/be/CMakeLists.txt File be/CMakeLists.txt: Line 73: if (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 7.0) Should this be limited to GCC, too? Line 74: # -Wno-placement-new: avoid spurious warnings from boost::function. Can you describe this error a bit more? Line 75: # -faligned-new: new will automatically align types. Otherwise "new Counter()" in the It is my understanding that new produces things that are as aligned as void * (aka 8 bytes). Can you describe a bit more what's going on with this warning? http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/exec/hbase-table-scanner.cc File be/src/exec/hbase-table-scanner.cc: Line 764: discard_result(JniUtil::FreeGlobalRef(env, resultscanner_)); What is the rationale for the difference between this line and 761, where you LOG(WARNING)? See also below. http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/exec/hbase-table-scanner.h File be/src/exec/hbase-table-scanner.h: Line 88: static Status Init() WARN_UNUSED_RESULT; Once we move to GCC7, we no longer need WARN_UNUSED_RESULT? http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/rpc/auth-provider.h File be/src/rpc/auth-provider.h: Line 51: /// that fix wrap http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/rpc/thrift-client.cc File be/src/rpc/thrift-client.cc: Line 40: if (!init_status_.ok()) return init_status_; RETURN_IF_ERROR? http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/rpc/thrift-client.h File be/src/rpc/thrift-client.h: Line 161: if (!init_status_.ok()) return; and LOG? http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/runtime/data-stream-recvr.cc File be/src/runtime/data-stream-recvr.cc: Line 346: Status status = mgr_->DeregisterRecvr(fragment_instance_id(), dest_node_id()); const http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: Line 100: discard_result(WaitForPrepare()); // make sure Prepare() finished Why is discard OK here? http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/runtime/hbase-table-factory.cc File be/src/runtime/hbase-table-factory.cc: Line 95: discard_result(JniUtil::FreeGlobalRef(env, connection_)); Why is discard OK here? http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/scheduling/scheduler-test-util.cc File be/src/scheduling/scheduler-test-util.cc: Line 510: Status status = scheduler_->Init(); const http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/service/fe-support.cc File be/src/service/fe-support.cc: Line 72: THROW_IF_ERROR(LlvmCodeGen::InitializeLlvm(true), env, JniUtil::internal_exc_class()); I'm surprised you want to throw in a function with extern "C" linkage. http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 697: Status status = profile_logger_->Flush(); const -- To view, visit http://gerrit.cloudera.org:8080/7253 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-HasComments: Yes
