impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <>
Subject [Impala-ASF-CR] PREVIEW: IMPALA-2615: support [[nodiscard]] on Status
Date Fri, 11 Aug 2017 00:02:04 GMT
Jim Apple has posted comments on this change.

Change subject: PREVIEW: IMPALA-2615: support [[nodiscard]] on Status

Patch Set 5:


Halfway through reviewing; though we should hash some of this out before I do the second half.
File be/CMakeLists.txt:

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?
File be/src/exec/

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.
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?
File be/src/rpc/auth-provider.h:

Line 51:   /// that
fix wrap
File be/src/rpc/

Line 40:   if (!init_status_.ok()) return init_status_;
File be/src/rpc/thrift-client.h:

Line 161:   if (!init_status_.ok()) return;
and LOG?
File be/src/runtime/

Line 346:   Status status = mgr_->DeregisterRecvr(fragment_instance_id(), dest_node_id());
File be/src/runtime/

Line 100:   discard_result(WaitForPrepare());  // make sure Prepare() finished
Why is discard OK here?
File be/src/runtime/

Line 95:     discard_result(JniUtil::FreeGlobalRef(env, connection_));
Why is discard OK here?
File be/src/scheduling/

Line 510:   Status status = scheduler_->Init();
File be/src/service/

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.
File be/src/service/

Line 697:     Status status = profile_logger_->Flush();

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-HasComments: Yes

View raw message