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 <tarmstr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org>
Gerrit-HasComments: Yes

Reply via email to